From 84f305915fd45c1e55d261db565b3ddfdbc9d4ce Mon Sep 17 00:00:00 2001 From: Geoffry Song Date: Wed, 5 Feb 2014 18:36:24 -0500 Subject: [PATCH 1/2] linux: always deregister closing fds from epoll If the same file description is open in two different processes, then closing the file descriptor is not sufficient to deregister it from the epoll instance (as described in epoll(7)), resulting in spurious events that cause the event loop to spin repeatedly. So always explicitly deregister it. Fixes #1099. Conflicts: test/test-spawn.c --- src/unix/linux-core.c | 20 ++++++++---- test/test-list.h | 2 ++ test/test-spawn.c | 74 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 90 insertions(+), 6 deletions(-) diff --git a/src/unix/linux-core.c b/src/unix/linux-core.c index 7a8fcd35..69a60b05 100644 --- a/src/unix/linux-core.c +++ b/src/unix/linux-core.c @@ -99,6 +99,7 @@ void uv__platform_loop_delete(uv_loop_t* loop) { void uv__platform_invalidate_fd(uv_loop_t* loop, int fd) { struct uv__epoll_event* events; + struct uv__epoll_event dummy; uintptr_t i; uintptr_t nfds; @@ -106,13 +107,20 @@ void uv__platform_invalidate_fd(uv_loop_t* loop, int fd) { events = (struct uv__epoll_event*) loop->watchers[loop->nwatchers]; nfds = (uintptr_t) loop->watchers[loop->nwatchers + 1]; - if (events == NULL) - return; + if (events != NULL) + /* Invalidate events with same file descriptor */ + for (i = 0; i < nfds; i++) + if ((int) events[i].data == fd) + events[i].data = -1; - /* Invalidate events with same file descriptor */ - for (i = 0; i < nfds; i++) - if ((int) events[i].data == fd) - events[i].data = -1; + /* Remove the file descriptor from the epoll. + * This avoids a problem where the same file description remains open + * in another process, causing repeated junk epoll events. + * + * We pass in a dummy epoll_event, to work around a bug in old kernels. + */ + if (loop->backend_fd >= 0) + uv__epoll_ctl(loop->backend_fd, UV__EPOLL_CTL_DEL, fd, &dummy); } diff --git a/test/test-list.h b/test/test-list.h index f60eab30..2d8942c3 100644 --- a/test/test-list.h +++ b/test/test-list.h @@ -226,6 +226,7 @@ TEST_DECLARE (spawn_setuid_setgid) TEST_DECLARE (we_get_signal) TEST_DECLARE (we_get_signals) TEST_DECLARE (signal_multiple_loops) +TEST_DECLARE (closed_fd_events) #endif #ifdef __APPLE__ TEST_DECLARE (osx_select) @@ -458,6 +459,7 @@ TASK_LIST_START TEST_ENTRY (we_get_signal) TEST_ENTRY (we_get_signals) TEST_ENTRY (signal_multiple_loops) + TEST_ENTRY (closed_fd_events) #endif #ifdef __APPLE__ diff --git a/test/test-spawn.c b/test/test-spawn.c index 43889878..337cd1d8 100644 --- a/test/test-spawn.c +++ b/test/test-spawn.c @@ -40,6 +40,7 @@ static char exepath[1024]; static size_t exepath_size = 1024; static char* args[3]; static int no_term_signal; +static int timer_counter; #define OUTPUT_SIZE 1024 static char output[OUTPUT_SIZE]; @@ -118,6 +119,12 @@ static void on_read(uv_stream_t* tcp, ssize_t nread, uv_buf_t buf) { } +static void on_read_once(uv_stream_t* tcp, ssize_t nread, const uv_buf_t buf) { + uv_read_stop(tcp); + on_read(tcp, nread, buf); +} + + static void write_cb(uv_write_t* req, int status) { ASSERT(status == 0); uv_close((uv_handle_t*)req->handle, close_cb); @@ -145,6 +152,11 @@ static void timer_cb(uv_timer_t* handle, int status) { } +static void timer_counter_cb(uv_timer_t* handle, int status) { + ++timer_counter; +} + + TEST_IMPL(spawn_fails) { init_process_options("", exit_cb_failure_expected); options.file = options.args[0] = "program-that-had-better-not-exist"; @@ -1038,3 +1050,65 @@ TEST_IMPL(spawn_auto_unref) { MAKE_VALGRIND_HAPPY(); return 0; } + + +#ifndef _WIN32 +TEST_IMPL(closed_fd_events) { + uv_stdio_container_t stdio[3]; + uv_pipe_t pipe_handle; + int fd[2]; + + /* create a pipe and share it with a child process */ + ASSERT(0 == pipe(fd)); + ASSERT(0 == fcntl(fd[0], F_SETFL, O_NONBLOCK)); + + /* spawn_helper4 blocks indefinitely. */ + init_process_options("spawn_helper4", exit_cb); + options.stdio_count = 3; + options.stdio = stdio; + options.stdio[0].data.fd = fd[0]; + options.stdio[0].flags = UV_INHERIT_FD; + options.stdio[1].flags = UV_IGNORE; + options.stdio[2].flags = UV_IGNORE; + + ASSERT(0 == uv_spawn(uv_default_loop(), &process, options)); + uv_unref((uv_handle_t*) &process); + + /* read from the pipe with uv */ + ASSERT(0 == uv_pipe_init(uv_default_loop(), &pipe_handle, 0)); + ASSERT(0 == uv_pipe_open(&pipe_handle, fd[0])); + fd[0] = -1; + + ASSERT(0 == uv_read_start((uv_stream_t*) &pipe_handle, on_alloc, on_read_once)); + + ASSERT(1 == write(fd[1], "", 1)); + + ASSERT(0 == uv_run(uv_default_loop(), UV_RUN_ONCE)); + + /* should have received just one byte */ + ASSERT(output_used == 1); + + /* close the pipe and see if we still get events */ + uv_close((uv_handle_t*) &pipe_handle, close_cb); + + ASSERT(1 == write(fd[1], "", 1)); + + ASSERT(0 == uv_timer_init(uv_default_loop(), &timer)); + ASSERT(0 == uv_timer_start(&timer, timer_counter_cb, 10, 0)); + + /* see if any spurious events interrupt the timer */ + if (1 == uv_run(uv_default_loop(), UV_RUN_ONCE)) { + if (1 == uv_run(uv_default_loop(), UV_RUN_ONCE)) + ASSERT(0 == uv_run(uv_default_loop(), UV_RUN_ONCE)); + } + + ASSERT(timer_counter == 1); + + /* cleanup */ + ASSERT(0 == uv_process_kill(&process, /* SIGTERM */ 15)); + ASSERT(0 == close(fd[1])); + + MAKE_VALGRIND_HAPPY(); + return 0; +} +#endif /* !_WIN32 */ From 9b38c01b540b60408f8eb1d9c288656405e25f7e Mon Sep 17 00:00:00 2001 From: Fedor Indutny Date: Tue, 11 Mar 2014 01:44:44 +0400 Subject: [PATCH 2/2] kqueue: invalidate fd in uv_fs_event_t Invalidate file descriptor when closing `uv_fs_event_t` handle. Note that `uv__io_stop` is just removing `fd` from `loop->watchers` and not actually invalidating all consequent events in a `kevent()` results. fix joyent/node#1101 --- src/unix/kqueue.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/unix/kqueue.c b/src/unix/kqueue.c index dc642034..124f4fd5 100644 --- a/src/unix/kqueue.c +++ b/src/unix/kqueue.c @@ -359,10 +359,10 @@ fallback: void uv__fs_event_close(uv_fs_event_t* handle) { #if defined(__APPLE__) if (uv__fsevents_close(handle)) - uv__io_stop(handle->loop, &handle->event_watcher, UV__POLLIN); -#else - uv__io_stop(handle->loop, &handle->event_watcher, UV__POLLIN); #endif /* defined(__APPLE__) */ + { + uv__io_close(handle->loop, &handle->event_watcher); + } uv__handle_stop(handle);