From 143da93e2d3f5d27ff4cc45b5a50a438f5cd7a6f Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Mon, 29 Oct 2018 12:44:40 +0100 Subject: [PATCH] test,unix: fix race in test runner The test runner inserted a 250 ms delay to give helper processes time to settle. That's intrinsically race-y and caused tests to intermittently fail on platforms like AIX. Instead of a fixed delay, pass a file descriptor to the helper process and wait until it closes the descriptor. That way we know for sure the process has started. Incidentally, this change reduces the running time of the test suite from 112 to 26 seconds on my machine. Fixes: https://github.com/libuv/libuv/issues/2041 PR-URL: https://github.com/libuv/libuv/pull/2056 Reviewed-By: Gireesh Punathil Reviewed-By: Santiago Gimeno --- test/echo-server.c | 4 ++ test/run-tests.c | 15 +++++ test/runner-unix.c | 64 ++++++++++++++++++++++ test/runner-win.c | 5 ++ test/runner.c | 3 - test/task.h | 6 ++ test/test-ipc-heavy-traffic-deadlock-bug.c | 1 + test/test-ipc-send-recv.c | 1 + test/test-ipc.c | 1 + test/test-stdio-over-pipes.c | 1 + 10 files changed, 98 insertions(+), 3 deletions(-) diff --git a/test/echo-server.c b/test/echo-server.c index bfed6767..a38e975d 100644 --- a/test/echo-server.c +++ b/test/echo-server.c @@ -340,6 +340,7 @@ HELPER_IMPL(tcp4_echo_server) { if (tcp4_echo_start(TEST_PORT)) return 1; + notify_parent_process(); uv_run(loop, UV_RUN_DEFAULT); return 0; } @@ -351,6 +352,7 @@ HELPER_IMPL(tcp6_echo_server) { if (tcp6_echo_start(TEST_PORT)) return 1; + notify_parent_process(); uv_run(loop, UV_RUN_DEFAULT); return 0; } @@ -362,6 +364,7 @@ HELPER_IMPL(pipe_echo_server) { if (pipe_echo_start(TEST_PIPENAME)) return 1; + notify_parent_process(); uv_run(loop, UV_RUN_DEFAULT); return 0; } @@ -373,6 +376,7 @@ HELPER_IMPL(udp4_echo_server) { if (udp4_echo_start(TEST_PORT)) return 1; + notify_parent_process(); uv_run(loop, UV_RUN_DEFAULT); return 0; } diff --git a/test/run-tests.c b/test/run-tests.c index 9b8af046..42bde0bb 100644 --- a/test/run-tests.c +++ b/test/run-tests.c @@ -109,20 +109,24 @@ static int maybe_run_test(int argc, char **argv) { } if (strcmp(argv[1], "spawn_helper1") == 0) { + notify_parent_process(); return 1; } if (strcmp(argv[1], "spawn_helper2") == 0) { + notify_parent_process(); printf("hello world\n"); return 1; } if (strcmp(argv[1], "spawn_tcp_server_helper") == 0) { + notify_parent_process(); return spawn_tcp_server_helper(); } if (strcmp(argv[1], "spawn_helper3") == 0) { char buffer[256]; + notify_parent_process(); ASSERT(buffer == fgets(buffer, sizeof(buffer) - 1, stdin)); buffer[sizeof(buffer) - 1] = '\0'; fputs(buffer, stdout); @@ -130,12 +134,14 @@ static int maybe_run_test(int argc, char **argv) { } if (strcmp(argv[1], "spawn_helper4") == 0) { + notify_parent_process(); /* Never surrender, never return! */ while (1) uv_sleep(10000); } if (strcmp(argv[1], "spawn_helper5") == 0) { const char out[] = "fourth stdio!\n"; + notify_parent_process(); #ifdef _WIN32 DWORD bytes; WriteFile((HANDLE) _get_osfhandle(3), out, sizeof(out) - 1, &bytes, NULL); @@ -156,6 +162,8 @@ static int maybe_run_test(int argc, char **argv) { if (strcmp(argv[1], "spawn_helper6") == 0) { int r; + notify_parent_process(); + r = fprintf(stdout, "hello world\n"); ASSERT(r > 0); @@ -168,6 +176,9 @@ static int maybe_run_test(int argc, char **argv) { if (strcmp(argv[1], "spawn_helper7") == 0) { int r; char *test; + + notify_parent_process(); + /* Test if the test value from the parent is still set */ test = getenv("ENV_TEST"); ASSERT(test != NULL); @@ -181,6 +192,8 @@ static int maybe_run_test(int argc, char **argv) { #ifndef _WIN32 if (strcmp(argv[1], "spawn_helper8") == 0) { int fd; + + notify_parent_process(); ASSERT(sizeof(fd) == read(0, &fd, sizeof(fd))); ASSERT(fd > 2); ASSERT(-1 == write(fd, "x", 1)); @@ -190,6 +203,7 @@ static int maybe_run_test(int argc, char **argv) { #endif /* !_WIN32 */ if (strcmp(argv[1], "spawn_helper9") == 0) { + notify_parent_process(); return spawn_stdin_stdout(); } @@ -200,6 +214,7 @@ static int maybe_run_test(int argc, char **argv) { ASSERT(uid == getuid()); ASSERT(gid == getgid()); + notify_parent_process(); return 1; } diff --git a/test/runner-unix.c b/test/runner-unix.c index 6506fb3a..432cf33d 100644 --- a/test/runner-unix.c +++ b/test/runner-unix.c @@ -42,6 +42,30 @@ extern char** environ; +static void closefd(int fd) { + if (close(fd) == 0 || errno == EINTR || errno == EINPROGRESS) + return; + + perror("close"); + abort(); +} + + +void notify_parent_process(void) { + char* arg; + int fd; + + arg = getenv("UV_TEST_RUNNER_FD"); + if (arg == NULL) + return; + + fd = atoi(arg); + assert(fd > STDERR_FILENO); + unsetenv("UV_TEST_RUNNER_FD"); + closefd(fd); +} + + /* Do platform-specific initialization. */ int platform_init(int argc, char **argv) { /* Disable stdio output buffering. */ @@ -65,6 +89,9 @@ int process_start(char* name, char* part, process_info_t* p, int is_helper) { int stdout_fd; const char* arg; char* args[16]; + int pipefd[2]; + char fdstr[8]; + ssize_t rc; int n; pid_t pid; @@ -94,6 +121,19 @@ int process_start(char* name, char* part, process_info_t* p, int is_helper) { return -1; } + if (is_helper) { + if (pipe(pipefd)) { + perror("pipe"); + return -1; + } + + snprintf(fdstr, sizeof(fdstr), "%d", pipefd[1]); + if (setenv("UV_TEST_RUNNER_FD", fdstr, /* overwrite */ 1)) { + perror("setenv"); + return -1; + } + } + p->terminated = 0; p->status = 0; @@ -106,6 +146,8 @@ int process_start(char* name, char* part, process_info_t* p, int is_helper) { if (pid == 0) { /* child */ + if (is_helper) + closefd(pipefd[0]); dup2(stdout_fd, STDOUT_FILENO); dup2(stdout_fd, STDERR_FILENO); execve(args[0], args, environ); @@ -118,6 +160,28 @@ int process_start(char* name, char* part, process_info_t* p, int is_helper) { p->name = strdup(name); p->stdout_file = stdout_file; + if (!is_helper) + return 0; + + closefd(pipefd[1]); + unsetenv("UV_TEST_RUNNER_FD"); + + do + rc = read(pipefd[0], &n, 1); + while (rc == -1 && errno == EINTR); + + closefd(pipefd[0]); + + if (rc == -1) { + perror("read"); + return -1; + } + + if (rc > 0) { + fprintf(stderr, "EOF expected but got data.\n"); + return -1; + } + return 0; } diff --git a/test/runner-win.c b/test/runner-win.c index aa52d7cc..e3e91a7b 100644 --- a/test/runner-win.c +++ b/test/runner-win.c @@ -76,6 +76,11 @@ int process_start(char *name, char *part, process_info_t *p, int is_helper) { PROCESS_INFORMATION pi; DWORD result; + if (!is_helper) { + /* Give the helpers time to settle. Race-y, fix this. */ + uv_sleep(250); + } + if (GetTempPathW(sizeof(path) / sizeof(WCHAR), (WCHAR*)&path) == 0) goto error; if (GetTempFileNameW((WCHAR*)&path, L"uv", 0, (WCHAR*)&filename) == 0) diff --git a/test/runner.c b/test/runner.c index f017902a..aec560a5 100644 --- a/test/runner.c +++ b/test/runner.c @@ -215,9 +215,6 @@ int run_test(const char* test, process_count++; } - /* Give the helpers time to settle. Race-y, fix this. */ - uv_sleep(250); - /* Now start the test itself. */ for (task = TASKS; task->main; task++) { if (strcmp(test, task->task_name) != 0) { diff --git a/test/task.h b/test/task.h index 92a90a54..282c02d5 100644 --- a/test/task.h +++ b/test/task.h @@ -181,6 +181,12 @@ extern int snprintf(char*, size_t, const char*, ...); # define UNUSED #endif +#if defined(_WIN32) +#define notify_parent_process() ((void) 0) +#else +extern void notify_parent_process(void); +#endif + /* Fully close a loop */ static void close_walk_cb(uv_handle_t* handle, void* arg) { if (!uv_is_closing(handle)) diff --git a/test/test-ipc-heavy-traffic-deadlock-bug.c b/test/test-ipc-heavy-traffic-deadlock-bug.c index 240fc645..325305a6 100644 --- a/test/test-ipc-heavy-traffic-deadlock-bug.c +++ b/test/test-ipc-heavy-traffic-deadlock-bug.c @@ -150,6 +150,7 @@ int ipc_helper_heavy_traffic_deadlock_bug(void) { r = uv_pipe_open(&pipe, 0); ASSERT(r == 0); + notify_parent_process(); do_writes_and_reads((uv_stream_t*) &pipe); uv_sleep(100); diff --git a/test/test-ipc-send-recv.c b/test/test-ipc-send-recv.c index 3dedc86b..166225c0 100644 --- a/test/test-ipc-send-recv.c +++ b/test/test-ipc-send-recv.c @@ -397,6 +397,7 @@ int run_ipc_send_recv_helper(uv_loop_t* loop, int inprocess) { send_recv_start(); } + notify_parent_process(); r = uv_run(loop, UV_RUN_DEFAULT); ASSERT(r == 0); diff --git a/test/test-ipc.c b/test/test-ipc.c index 200f68d6..829d178d 100644 --- a/test/test-ipc.c +++ b/test/test-ipc.c @@ -724,6 +724,7 @@ int ipc_helper(int listen_after_write) { ASSERT(r == 0); } + notify_parent_process(); r = uv_run(uv_default_loop(), UV_RUN_DEFAULT); ASSERT(r == 0); diff --git a/test/test-stdio-over-pipes.c b/test/test-stdio-over-pipes.c index 15744761..a130ff6a 100644 --- a/test/test-stdio-over-pipes.c +++ b/test/test-stdio-over-pipes.c @@ -232,6 +232,7 @@ int stdio_over_pipes_helper(void) { ASSERT(r == 0); } + notify_parent_process(); uv_run(loop, UV_RUN_DEFAULT); ASSERT(after_write_called == 7);