Run compile tests in a way that's safe in a multithreaded environment

In theory. There's still something leaking but I hope it's no longer due to
the test runner doing unsafe things.

This is a pretty nasty patch, working around limitations in the type and task
systems, and it makes the std::test API a little uglier.

src/lib/test.rs

@@ -16,9 +16,12 @@ export tr_ok;
 export tr_failed;
 export tr_ignored;
 export run_tests_console;
+export run_tests_console_;
 export run_test;
 export filter_tests;
 export parse_opts;
+export test_to_task;
+export default_test_to_task;
 
 // The name of a test. By convention this follows the rules for rust
 // paths, i.e it should be a series of identifiers seperated by double
@@ -95,8 +98,21 @@ tag test_result {
     tr_ignored;
 }
 
+// To get isolation and concurrency tests have to be run in their own tasks.
+// In cases where test functions and closures it is not ok to just dump them
+// into a task and run them, so this transformation gives the caller a chance
+// to create the test task.
+type test_to_task = fn(&fn()) -> task;
+
 // A simple console test runner
-fn run_tests_console(&test_opts opts, &test_desc[] tests) -> bool {
+fn run_tests_console(&test_opts opts,
+                     &test_desc[] tests) -> bool {
+    run_tests_console_(opts, tests, default_test_to_task)
+}
+
+fn run_tests_console_(&test_opts opts,
+                     &test_desc[] tests,
+                     &test_to_task to_task) -> bool {
 
     auto filtered_tests = filter_tests(opts, tests);
 
@@ -124,7 +140,7 @@ fn run_tests_console(&test_opts opts, &test_desc[] tests) -> bool {
     while (wait_idx < total) {
         while (ivec::len(futures) < concurrency
                && run_idx < total) {
-            futures += ~[run_test(filtered_tests.(run_idx))];
+            futures += ~[run_test(filtered_tests.(run_idx), to_task)];
             run_idx += 1u;
         }
 
@@ -266,14 +282,14 @@ type test_future = rec(test_desc test,
                        @fn() fnref,
                        fn() -> test_result wait);
 
-fn run_test(&test_desc test) -> test_future {
+fn run_test(&test_desc test, &test_to_task to_task) -> test_future {
     // FIXME: Because of the unsafe way we're passing the test function
     // to the test task, we need to make sure we keep a reference to that
     // function around for longer than the lifetime of the task. To that end
     // we keep the function boxed in the test future.
     auto fnref = @test.fn;
     if (!test.ignore) {
-        auto test_task = run_test_fn_in_task(*fnref);
+        auto test_task = to_task(*fnref);
         ret rec(test = test,
                 fnref = fnref,
                 wait = bind fn(&task test_task) -> test_result {
@@ -295,8 +311,9 @@ native "rust" mod rustrt {
 
 // We need to run our tests in another task in order to trap test failures.
 // But, at least currently, functions can't be used as spawn arguments so
-// we've got to treat our test functions as unsafe pointers.
+// we've got to treat our test functions as unsafe pointers.  This function
-fn run_test_fn_in_task(&fn() f) -> task {
+// only works with functions that don't contain closures.
+fn default_test_to_task(&fn() f) -> task {
     fn run_task(*mutable fn() fptr) {
         // If this task fails we don't want that failure to propagate to the
         // test runner or else we couldn't keep running tests

src/test/compiletest/compiletest.rs

@@ -87,12 +87,7 @@ fn parse_config(&str[] args) -> config {
             src_base = getopts::opt_str(match, "src-base"),
             build_base = getopts::opt_str(match, "build-base"),
             stage_id = getopts::opt_str(match, "stage-id"),
-            mode = alt getopts::opt_str(match, "mode") {
+            mode = str_mode(getopts::opt_str(match, "mode")),
-                "compile-fail" { mode_compile_fail }
-                "run-fail" { mode_run_fail }
-                "run-pass" { mode_run_pass }
-                _ { fail "invalid mode" }
-            },
             run_ignored = getopts::opt_present(match, "ignored"),
             filter = if vec::len(match.free) > 0u {
                 option::some(match.free.(0))
@@ -115,22 +110,37 @@ fn log_config(&config config) {
     logv(c, #fmt("stage_id: %s", config.stage_id));
     logv(c, #fmt("mode: %s", mode_str(config.mode)));
     logv(c, #fmt("run_ignored: %b", config.run_ignored));
-    logv(c, #fmt("filter: %s", alt (config.filter) {
+    logv(c, #fmt("filter: %s", opt_str(config.filter)));
-      option::some(?f) { f }
+    logv(c, #fmt("runtool: %s", opt_str(config.runtool)));
-      option::none { "(none)" }
+    logv(c, #fmt("rustcflags: %s", opt_str(config.rustcflags)));
-    }));
-    logv(c, #fmt("runtool: %s", alt (config.runtool) {
-      option::some(?s) { s }
-      option::none { "(none)" }
-    }));
-    logv(c, #fmt("rustcflags: %s", alt (config.rustcflags) {
-      option::some(?s) { s }
-      option::none { "(none)" }
-    }));
     logv(c, #fmt("verbose: %b", config.verbose));
     logv(c, #fmt("\n"));
 }
 
+fn opt_str(option::t[str] maybestr) -> str {
+    alt maybestr {
+      option::some(?s) { s }
+      option::none { "(none)" }
+    }
+}
+
+fn str_opt(str maybestr) -> option::t[str] {
+    if maybestr != "(none)" {
+        option::some(maybestr)
+    } else {
+        option::none
+    }
+}
+
+fn str_mode(str s) -> mode {
+    alt s {
+      "compile-fail" { mode_compile_fail }
+      "run-fail" { mode_run_fail }
+      "run-pass" { mode_run_pass }
+      _ { fail "invalid mode" }
+    }
+}
+
 fn mode_str(mode mode) -> str {
     alt (mode) {
         mode_compile_fail { "compile-fail" }
@@ -147,7 +157,7 @@ fn run_tests(&config config) {
     auto cx = rec(config = config,
                   procsrv = procsrv::mk());
     auto tests = make_tests(cx);
-    test::run_tests_console(opts, tests);
+    test::run_tests_console_(opts, tests.tests, tests.to_task);
     procsrv::close(cx.procsrv);
 }
 
@@ -156,16 +166,21 @@ fn test_opts(&config config) -> test::test_opts {
         run_ignored = config.run_ignored)
 }
 
-fn make_tests(&cx cx) -> test::test_desc[] {
+type tests_and_conv_fn = rec(test::test_desc[] tests,
+                             fn(&fn()) -> task to_task);
+
+fn make_tests(&cx cx) -> tests_and_conv_fn {
     log #fmt("making tests from %s", cx.config.src_base);
+    auto configport = port[str]();
     auto tests = ~[];
     for (str file in fs::list_dir(cx.config.src_base)) {
         log #fmt("inspecting file %s", file);
         if (is_test(file)) {
-            tests += ~[make_test(cx, file)];
+            tests += ~[make_test(cx, file, configport)];
         }
     }
-    ret tests;
+    ret rec(tests = tests,
+            to_task = bind closure_to_task(cx, configport, _));
 }
 
 fn is_test(&str testfile) -> bool {
@@ -176,9 +191,10 @@ fn is_test(&str testfile) -> bool {
          || str::starts_with(name, "~"))
 }
 
-fn make_test(&cx cx, &str testfile) -> test::test_desc {
+fn make_test(&cx cx, &str testfile,
+             &port[str] configport) -> test::test_desc {
     rec(name = testfile,
-        fn = make_test_fn(cx, testfile),
+        fn = make_test_closure(testfile, chan(configport)),
         ignore = is_test_ignored(cx.config, testfile))
 }
 
@@ -206,44 +222,97 @@ iter iter_header(&str testfile) -> str {
     }
 }
 
-fn make_test_fn(&cx cx, &str testfile) -> test::test_fn {
+/*
-    // We're doing some ferociously unsafe nonsense here by creating a closure
+So this is kind of crappy:
-    // and letting the test runner spawn it into a task. To avoid having
-    // different tasks fighting over their refcounts and then the wrong task
-    // freeing a box we need to clone everything, and make sure our closure
-    // outlives all the tasks.
-    fn clonestr(&str s) -> str {
-        str::unsafe_from_bytes(str::bytes(s))
-    }
 
-    fn cloneoptstr(&option::t[str] s) -> option::t[str] {
+A test is just defined as a function, as you might expect, but tests have to
-        alt s {
+run their own tasks. Unfortunately, if your test needs dynamic data then it
-          option::some(?s) { option::some(clonestr(s)) }
+needs to be a closure, and transferring closures across tasks without
-          option::none { option::none }
+committing a host of memory management transgressions is just impossible.
-        }
-    }
 
-    auto configclone = rec(
+To get around this, the standard test runner allows you the opportunity do
-        compile_lib_path = clonestr(cx.config.compile_lib_path),
+your own conversion from a test function to a task. It gives you your function
-        run_lib_path = clonestr(cx.config.run_lib_path),
+and you give it back a task.
-        rustc_path = clonestr(cx.config.rustc_path),
+
-        src_base = clonestr(cx.config.src_base),
+So that's what we're going to do. Here's where it gets stupid. To get the
-        build_base = clonestr(cx.config.build_base),
+the data out of the test function we are going to run the test function,
-        stage_id = clonestr(cx.config.stage_id),
+which will do nothing but send the data for that test to a port we've set
-        mode = cx.config.mode,
+up. Then we'll spawn that data into another task and return the task.
-        run_ignored = cx.config.run_ignored,
+Really convoluted. Need to think up of a better definition for tests.
-        filter = cloneoptstr(cx.config.filter),
+*/
-        runtool = cloneoptstr(cx.config.runtool),
+
-        rustcflags = cloneoptstr(cx.config.rustcflags),
+fn make_test_closure(&str testfile,
-        verbose = cx.config.verbose);
+                     chan[str] configchan) -> test::test_fn {
-    auto cxclone = rec(config = configclone,
+    bind send_config(testfile, configchan)
-                       procsrv = procsrv::clone(cx.procsrv));
-    auto testfileclone = clonestr(testfile);
-    ret bind run_test(cxclone, testfileclone);
 }
 
-fn run_test(cx cx, str testfile) {
+fn send_config(str testfile, chan[str] configchan) {
+    task::send(configchan, testfile);
+}
+
+/*
+FIXME: Good god forgive me.
+
+So actually shuttling structural data across tasks isn't possible at this
+time, but we can send strings! Sadly, I need the whole config record, in the
+test task so, instead of fixing the mechanism in the compiler I'm going to
+break up the config record and pass everything individually to the spawned
+function.  */
+
+fn closure_to_task(cx cx, port[str] configport, &fn() testfn) -> task{
+    testfn();
+    auto testfile = task::recv(configport);
+    ret spawn run_test_task(cx.config.compile_lib_path,
+                            cx.config.run_lib_path,
+                            cx.config.rustc_path,
+                            cx.config.src_base,
+                            cx.config.build_base,
+                            cx.config.stage_id,
+                            mode_str(cx.config.mode),
+                            cx.config.run_ignored,
+                            opt_str(cx.config.filter),
+                            opt_str(cx.config.runtool),
+                            opt_str(cx.config.rustcflags),
+                            cx.config.verbose,
+                            procsrv::clone(cx.procsrv).chan,
+                            testfile);
+}
+
+fn run_test_task(str compile_lib_path,
+                 str run_lib_path,
+                 str rustc_path,
+                 str src_base,
+                 str build_base,
+                 str stage_id,
+                 str mode,
+                 bool run_ignored,
+                 str opt_filter,
+                 str opt_runtool,
+                 str opt_rustcflags,
+                 bool verbose,
+                 procsrv::reqchan procsrv_chan,
+                 str testfile) {
+
+    auto config = rec(compile_lib_path = compile_lib_path,
+                      run_lib_path = run_lib_path,
+                      rustc_path = rustc_path,
+                      src_base = src_base,
+                      build_base = build_base,
+                      stage_id = stage_id,
+                      mode = str_mode(mode),
+                      run_ignored = run_ignored,
+                      filter = str_opt(opt_filter),
+                      runtool = str_opt(opt_runtool),
+                      rustcflags = str_opt(opt_rustcflags),
+                      verbose = verbose);
+
+    auto procsrv = procsrv::from_chan(procsrv_chan);
+
+    auto cx = rec(config = config,
+                  procsrv = procsrv);
+
     log #fmt("running %s", testfile);
+    task::unsupervise();
     auto props = load_props(testfile);
     alt (cx.config.mode) {
         mode_compile_fail {
@@ -561,12 +630,16 @@ mod procsrv {
 
     export handle;
     export mk;
+    export from_chan;
     export clone;
     export run;
     export close;
+    export reqchan;
+
+    type reqchan = chan[request];
 
     type handle = rec(option::t[task] task,
-                      chan[request] chan);
+                      reqchan chan);
 
     tag request {
         exec(str, str, vec[str], chan[response]);
@@ -581,6 +654,11 @@ mod procsrv {
                 chan = res.chan);
     }
 
+    fn from_chan(&reqchan ch) -> handle {
+        rec(task = option::none,
+            chan = ch)
+    }
+
     fn clone(&handle handle) -> handle {
         // Sharing tasks across tasks appears to be (yet another) recipe for
         // disaster, so our handle clones will not get the task pointer.

src/test/stdtest/test.rs

@@ -15,7 +15,7 @@ fn do_not_run_ignored_tests() {
                     fn = f,
                     ignore = true);
 
-    test::run_test(desc);
+    test::run_test(desc, test::default_test_to_task);
 
     assert ran == false;
 }
@@ -26,7 +26,7 @@ fn ignored_tests_result_in_ignored() {
     auto desc = rec(name = "whatever",
                     fn = f,
                     ignore = true);
-    auto res = test::run_test(desc).wait();
+    auto res = test::run_test(desc, test::default_test_to_task).wait();
     assert res == test::tr_ignored;
 }