From bcb5c4d54f601d6066d6fa463390d3d8b30f53b7 Mon Sep 17 00:00:00 2001 From: Brian Anderson Date: Tue, 26 Jul 2011 18:34:29 -0700 Subject: [PATCH] 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 | 29 ++++- src/test/compiletest/compiletest.rs | 194 +++++++++++++++++++--------- src/test/stdtest/test.rs | 4 +- 3 files changed, 161 insertions(+), 66 deletions(-) diff --git a/src/lib/test.rs b/src/lib/test.rs index d46c84eda79f..bfbbdee2705f 100644 --- a/src/lib/test.rs +++ b/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. -fn run_test_fn_in_task(&fn() f) -> task { +// we've got to treat our test functions as unsafe pointers. This function +// 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 diff --git a/src/test/compiletest/compiletest.rs b/src/test/compiletest/compiletest.rs index fec38cbb7564..cd349c378248 100644 --- a/src/test/compiletest/compiletest.rs +++ b/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") { - "compile-fail" { mode_compile_fail } - "run-fail" { mode_run_fail } - "run-pass" { mode_run_pass } - _ { fail "invalid mode" } - }, + mode = str_mode(getopts::opt_str(match, "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) { - option::some(?f) { f } - option::none { "(none)" } - })); - 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("filter: %s", opt_str(config.filter))); + logv(c, #fmt("runtool: %s", opt_str(config.runtool))); + logv(c, #fmt("rustcflags: %s", opt_str(config.rustcflags))); 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 - // 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)) - } +/* +So this is kind of crappy: - fn cloneoptstr(&option::t[str] s) -> option::t[str] { - alt s { - option::some(?s) { option::some(clonestr(s)) } - option::none { option::none } - } - } +A test is just defined as a function, as you might expect, but tests have to +run their own tasks. Unfortunately, if your test needs dynamic data then it +needs to be a closure, and transferring closures across tasks without +committing a host of memory management transgressions is just impossible. - auto configclone = rec( - compile_lib_path = clonestr(cx.config.compile_lib_path), - run_lib_path = clonestr(cx.config.run_lib_path), - rustc_path = clonestr(cx.config.rustc_path), - src_base = clonestr(cx.config.src_base), - build_base = clonestr(cx.config.build_base), - stage_id = clonestr(cx.config.stage_id), - mode = cx.config.mode, - run_ignored = cx.config.run_ignored, - filter = cloneoptstr(cx.config.filter), - runtool = cloneoptstr(cx.config.runtool), - rustcflags = cloneoptstr(cx.config.rustcflags), - verbose = cx.config.verbose); - auto cxclone = rec(config = configclone, - procsrv = procsrv::clone(cx.procsrv)); - auto testfileclone = clonestr(testfile); - ret bind run_test(cxclone, testfileclone); +To get around this, the standard test runner allows you the opportunity do +your own conversion from a test function to a task. It gives you your function +and you give it back a task. + +So that's what we're going to do. Here's where it gets stupid. To get the +the data out of the test function we are going to run the test function, +which will do nothing but send the data for that test to a port we've set +up. Then we'll spawn that data into another task and return the task. +Really convoluted. Need to think up of a better definition for tests. +*/ + +fn make_test_closure(&str testfile, + chan[str] configchan) -> test::test_fn { + bind send_config(testfile, configchan) } -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. diff --git a/src/test/stdtest/test.rs b/src/test/stdtest/test.rs index ba69674b0300..65c1a049f930 100644 --- a/src/test/stdtest/test.rs +++ b/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; }