diff options
| -rw-r--r-- | changes/bug13291-spawn-test-race-condition | 4 | ||||
| -rw-r--r-- | src/test/test_util.c | 62 |
2 files changed, 61 insertions, 5 deletions
diff --git a/changes/bug13291-spawn-test-race-condition b/changes/bug13291-spawn-test-race-condition new file mode 100644 index 0000000000..c14de8a201 --- /dev/null +++ b/changes/bug13291-spawn-test-race-condition @@ -0,0 +1,4 @@ + o Minor bugfixes: + - Stop spawn test failures due to a race condition between the SIGCHLD + handler updating the process status, and the test reading it. + Fixes bug 13291. diff --git a/src/test/test_util.c b/src/test/test_util.c index 9d9b393a27..7acda6bae7 100644 --- a/src/test/test_util.c +++ b/src/test/test_util.c @@ -2850,6 +2850,30 @@ test_util_fgets_eagain(void *ptr) #define EOL "\n" #endif +#ifdef _WIN32 +/* I've assumed Windows doesn't have the gap between fork and exec + * that causes the race condition on unix-like platforms */ +#define MATCH_PROCESS_STATUS(s1,s2) (s1 == s2) + +#else +/* work around a race condition of the timing of SIGCHLD handler updates + * to the process_handle's fields, and checks of those fields + * + * TODO: Once we can signal failure to exec, change PROCESS_STATUS_RUNNING to + * PROCESS_STATUS_ERROR (and similarly with *_OR_NOTRUNNING) */ +#define PROCESS_STATUS_RUNNING_OR_NOTRUNNING (PROCESS_STATUS_RUNNING+1) +#define IS_RUNNING_OR_NOTRUNNING(s) \ + (s == PROCESS_STATUS_RUNNING || s == PROCESS_STATUS_NOTRUNNING) +/* well, this is ugly */ +#define MATCH_PROCESS_STATUS(s1,s2) \ + ( s1 == s2 \ + ||(s1 == PROCESS_STATUS_RUNNING_OR_NOTRUNNING \ + && IS_RUNNING_OR_NOTRUNNING(s2)) \ + ||(s2 == PROCESS_STATUS_RUNNING_OR_NOTRUNNING \ + && IS_RUNNING_OR_NOTRUNNING(s1))) + +#endif // _WIN32 + /** Helper function for testing tor_spawn_background */ static void run_util_spawn_background(const char *argv[], const char *expected_out, @@ -2871,18 +2895,39 @@ run_util_spawn_background(const char *argv[], const char *expected_out, notify_pending_waitpid_callbacks(); - tt_int_op(expected_status,==, status); + /* the race condition doesn't affect status, + * because status isn't updated by the SIGCHLD handler, + * but we still need to handle PROCESS_STATUS_RUNNING_OR_NOTRUNNING */ + tt_assert(MATCH_PROCESS_STATUS(expected_status, status)); if (status == PROCESS_STATUS_ERROR) { tt_ptr_op(process_handle, ==, NULL); return; } tt_assert(process_handle != NULL); - tt_int_op(expected_status,==, process_handle->status); + + /* When a spawned process forks, fails, then exits very quickly, + * (this typically occurs when exec fails) + * there is a race condition between the SIGCHLD handler + * updating the process_handle's fields, and this test + * checking the process status in those fields. + * The SIGCHLD update can occur before or after the code below executes. + * This causes intermittent failures in spawn_background_fail(), + * typically when the machine is under load. + * We use PROCESS_STATUS_RUNNING_OR_NOTRUNNING to avoid this issue. */ + + /* the race condition affects the change in + * process_handle->status from RUNNING to NOTRUNNING */ + tt_assert(MATCH_PROCESS_STATUS(expected_status, process_handle->status)); #ifndef _WIN32 notify_pending_waitpid_callbacks(); - tt_ptr_op(process_handle->waitpid_cb, !=, NULL); + /* the race condition affects the change in + * process_handle->waitpid_cb to NULL, + * so we skip the check if expected_status is ambiguous, + * that is, PROCESS_STATUS_RUNNING_OR_NOTRUNNING */ + tt_assert(process_handle->waitpid_cb != NULL + || expected_status == PROCESS_STATUS_RUNNING_OR_NOTRUNNING); #endif #ifdef _WIN32 @@ -2955,8 +3000,8 @@ test_util_spawn_background_fail(void *ptr) const int expected_status = PROCESS_STATUS_ERROR; #else /* TODO: Once we can signal failure to exec, set this to be - * PROCESS_STATUS_ERROR */ - const int expected_status = PROCESS_STATUS_RUNNING; + * PROCESS_STATUS_RUNNING_OR_ERROR */ + const int expected_status = PROCESS_STATUS_RUNNING_OR_NOTRUNNING; #endif memset(expected_out, 0xf0, sizeof(expected_out)); @@ -3149,6 +3194,13 @@ test_util_spawn_background_waitpid_notify(void *arg) #undef TEST_CHILD #undef EOL +#undef MATCH_PROCESS_STATUS + +#ifndef _WIN32 +#undef PROCESS_STATUS_RUNNING_OR_NOTRUNNING +#undef IS_RUNNING_OR_NOTRUNNING +#endif + /** * Test for format_hex_number_sigsafe() */ |