Skip to content

Commit

Permalink
fix: libuv patches to address child_process.spawn slowness
Browse files Browse the repository at this point in the history
  • Loading branch information
nornagon committed Mar 18, 2022
1 parent f2b0632 commit 322fe50
Show file tree
Hide file tree
Showing 7 changed files with 1,398 additions and 0 deletions.
6 changes: 6 additions & 0 deletions patches/node/.patches
Expand Up @@ -33,3 +33,9 @@ darwin_translate_eprototype_to_econnreset_3413.patch
darwin_bump_minimum_supported_version_to_10_15_3406.patch
fix_failing_node_js_test_on_outdated.patch
be_compatible_with_cppgc.patch
process_monitor_for_exit_with_kqueue_on_bsds_3441.patch
unix_protect_fork_in_uv_spawn_from_signals.patch
process_bsd_handle_kevent_note_exit_failure_3451.patch
reland_macos_use_posix_spawn_instead_of_fork_3257.patch
process_reset_the_signal_mask_if_the_fork_fails_3537.patch
process_only_use_f_dupfd_cloexec_if_it_is_defined_3512.patch
@@ -0,0 +1,70 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash@gmail.com>
Date: Tue, 1 Feb 2022 15:27:12 -0500
Subject: process,bsd: handle kevent NOTE_EXIT failure (#3451)

The kernel may return ESRCH if the child has already exited here.
This is rather annoying, and means we must indirectly handle
notification to our event loop of the process exit.

Refs: https://github.com/libuv/libuv/pull/3441
Refs: https://github.com/libuv/libuv/pull/3257

diff --git a/deps/uv/src/unix/internal.h b/deps/uv/src/unix/internal.h
index 16be13b99f5db77741aa276e90a437ef4eb5ba32..2dcc8b32f5165dd75061a1b55cc1abd2ab93ccc9 100644
--- a/deps/uv/src/unix/internal.h
+++ b/deps/uv/src/unix/internal.h
@@ -145,7 +145,8 @@ typedef struct uv__stream_queued_fds_s uv__stream_queued_fds_t;

/* loop flags */
enum {
- UV_LOOP_BLOCK_SIGPROF = 1
+ UV_LOOP_BLOCK_SIGPROF = 0x1,
+ UV_LOOP_REAP_CHILDREN = 0x2
};

/* flags of excluding ifaddr */
diff --git a/deps/uv/src/unix/kqueue.c b/deps/uv/src/unix/kqueue.c
index 35200f17495d80ed2d19ef9f6f76bbc92ee042f6..071fe0ce0938657d0fb840af62a432352e938a8a 100644
--- a/deps/uv/src/unix/kqueue.c
+++ b/deps/uv/src/unix/kqueue.c
@@ -285,7 +285,7 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {
for (i = 0; i < nfds; i++) {
ev = events + i;
if (ev->filter == EVFILT_PROC) {
- uv__wait_children(loop);
+ loop->flags |= UV_LOOP_REAP_CHILDREN;
nevents++;
continue;
}
@@ -383,6 +383,11 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {
nevents++;
}

+ if (loop->flags & UV_LOOP_REAP_CHILDREN) {
+ loop->flags &= ~UV_LOOP_REAP_CHILDREN;
+ uv__wait_children(loop);
+ }
+
if (reset_timeout != 0) {
timeout = user_timeout;
reset_timeout = 0;
diff --git a/deps/uv/src/unix/process.c b/deps/uv/src/unix/process.c
index c1f6bd4b0076f0835caf83c45a6a896e7ae5def9..2920b942962357827bae9bcad23af8333e8b007f 100644
--- a/deps/uv/src/unix/process.c
+++ b/deps/uv/src/unix/process.c
@@ -507,8 +507,12 @@ int uv_spawn(uv_loop_t* loop,
#if defined(__APPLE__) || defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__)
struct kevent event;
EV_SET(&event, pid, EVFILT_PROC, EV_ADD | EV_ONESHOT, NOTE_EXIT, 0, 0);
- if (kevent(loop->backend_fd, &event, 1, NULL, 0, NULL))
- abort();
+ if (kevent(loop->backend_fd, &event, 1, NULL, 0, NULL)) {
+ if (errno != ESRCH)
+ abort();
+ /* Process already exited. Call waitpid on the next loop iteration. */
+ loop->flags |= UV_LOOP_REAP_CHILDREN;
+ }
#endif

QUEUE_INSERT_TAIL(&loop->process_handles, &process->queue);
170 changes: 170 additions & 0 deletions patches/node/process_monitor_for_exit_with_kqueue_on_bsds_3441.patch
@@ -0,0 +1,170 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jeremy Rose <nornagon@nornagon.net>
Date: Mon, 31 Jan 2022 11:49:22 -0800
Subject: process: monitor for exit with kqueue on BSDs (#3441)

This adds a workaround for an xnu kernel bug that sometimes results in
SIGCHLD not being delivered. The workaround is to use kevent to listen
for EVFILT_PROC/NOTE_EXIT events instead of relying on SIGCHLD on *BSD.

Apple rdar: FB9529664
Refs: https://github.com/libuv/libuv/pull/3257

diff --git a/deps/uv/src/unix/internal.h b/deps/uv/src/unix/internal.h
index 12d4da93686e993830a7d09e74d08191fc808f4f..16be13b99f5db77741aa276e90a437ef4eb5ba32 100644
--- a/deps/uv/src/unix/internal.h
+++ b/deps/uv/src/unix/internal.h
@@ -282,6 +282,7 @@ uv_handle_type uv__handle_type(int fd);
FILE* uv__open_file(const char* path);
int uv__getpwuid_r(uv_passwd_t* pwd);
int uv__search_path(const char* prog, char* buf, size_t* buflen);
+void uv__wait_children(uv_loop_t* loop);

/* random */
int uv__random_devurandom(void* buf, size_t buflen);
diff --git a/deps/uv/src/unix/kqueue.c b/deps/uv/src/unix/kqueue.c
index bf183d5fdc0ba89913469a294322eef84bc4cee8..35200f17495d80ed2d19ef9f6f76bbc92ee042f6 100644
--- a/deps/uv/src/unix/kqueue.c
+++ b/deps/uv/src/unix/kqueue.c
@@ -284,6 +284,11 @@ void uv__io_poll(uv_loop_t* loop, int timeout) {
loop->watchers[loop->nwatchers + 1] = (void*) (uintptr_t) nfds;
for (i = 0; i < nfds; i++) {
ev = events + i;
+ if (ev->filter == EVFILT_PROC) {
+ uv__wait_children(loop);
+ nevents++;
+ continue;
+ }
fd = ev->ident;
/* Skip invalidated events, see uv__platform_invalidate_fd */
if (fd == -1)
diff --git a/deps/uv/src/unix/process.c b/deps/uv/src/unix/process.c
index f4aebb0490e198cd9adcadfeb6b006de479cc993..cfcba341e0e380ecd595e4b59e39c08a7b374a48 100644
--- a/deps/uv/src/unix/process.c
+++ b/deps/uv/src/unix/process.c
@@ -48,10 +48,20 @@ extern char **environ;
# include "zos-base.h"
#endif

+#if defined(__APPLE__) || defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__)
+#include <sys/event.h>
+#endif
+

+#if !(defined(__APPLE__) || defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__))
static void uv__chld(uv_signal_t* handle, int signum) {
+ assert(signum == SIGCHLD);
+ uv__wait_children(handle->loop);
+}
+#endif
+
+void uv__wait_children(uv_loop_t* loop) {
uv_process_t* process;
- uv_loop_t* loop;
int exit_status;
int term_signal;
int status;
@@ -60,10 +70,7 @@ static void uv__chld(uv_signal_t* handle, int signum) {
QUEUE* q;
QUEUE* h;

- assert(signum == SIGCHLD);
-
QUEUE_INIT(&pending);
- loop = handle->loop;

h = &loop->process_handles;
q = QUEUE_HEAD(h);
@@ -419,7 +426,9 @@ int uv_spawn(uv_loop_t* loop,
if (err)
goto error;

+#if !(defined(__APPLE__) || defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__))
uv_signal_start(&loop->child_watcher, uv__chld, SIGCHLD);
+#endif

/* Acquire write lock to prevent opening new fds in worker threads */
uv_rwlock_wrlock(&loop->cloexec_lock);
@@ -478,6 +487,13 @@ int uv_spawn(uv_loop_t* loop,

/* Only activate this handle if exec() happened successfully */
if (exec_errorno == 0) {
+#if defined(__APPLE__) || defined(__DragonFly__) || defined(__FreeBSD__) || defined(__NetBSD__) || defined(__OpenBSD__)
+ struct kevent event;
+ EV_SET(&event, pid, EVFILT_PROC, EV_ADD | EV_ONESHOT, NOTE_EXIT, 0, 0);
+ if (kevent(loop->backend_fd, &event, 1, NULL, 0, NULL))
+ abort();
+#endif
+
QUEUE_INSERT_TAIL(&loop->process_handles, &process->queue);
uv__handle_start(process);
}
diff --git a/deps/uv/test/test-list.h b/deps/uv/test/test-list.h
index 59b95da9ebe3464bd1f9ce1c534122b1f9e06636..58489c4be7b3a7b36d5b01a1f07d411ef3d99ae3 100644
--- a/deps/uv/test/test-list.h
+++ b/deps/uv/test/test-list.h
@@ -318,6 +318,7 @@ TEST_DECLARE (spawn_reads_child_path)
TEST_DECLARE (spawn_inherit_streams)
TEST_DECLARE (spawn_quoted_path)
TEST_DECLARE (spawn_tcp_server)
+TEST_DECLARE (spawn_exercise_sigchld_issue)
TEST_DECLARE (fs_poll)
TEST_DECLARE (fs_poll_getpath)
TEST_DECLARE (fs_poll_close_request)
@@ -944,6 +945,7 @@ TASK_LIST_START
TEST_ENTRY (spawn_inherit_streams)
TEST_ENTRY (spawn_quoted_path)
TEST_ENTRY (spawn_tcp_server)
+ TEST_ENTRY (spawn_exercise_sigchld_issue)
TEST_ENTRY (fs_poll)
TEST_ENTRY (fs_poll_getpath)
TEST_ENTRY (fs_poll_close_request)
diff --git a/deps/uv/test/test-spawn.c b/deps/uv/test/test-spawn.c
index 9f2eb24b2d6daf339bec12027134409cfc3b4a82..dfd5458ef37c664af9a55a8383bdb3121885db3b 100644
--- a/deps/uv/test/test-spawn.c
+++ b/deps/uv/test/test-spawn.c
@@ -1891,6 +1891,44 @@ TEST_IMPL(spawn_quoted_path) {
#endif
}

+TEST_IMPL(spawn_exercise_sigchld_issue) {
+ int r;
+ int i;
+ uv_process_options_t dummy_options = {0};
+ uv_process_t dummy_processes[100];
+ char* args[2];
+
+ init_process_options("spawn_helper1", exit_cb);
+
+ r = uv_spawn(uv_default_loop(), &process, &options);
+ ASSERT_EQ(r, 0);
+
+ // This test exercises a bug in the darwin kernel that causes SIGCHLD not to
+ // be delivered sometimes. Calling posix_spawn many times increases the
+ // likelihood of encountering this issue, so spin a few times to make this
+ // test more reliable.
+ dummy_options.file = args[0] = "program-that-had-better-not-exist";
+ args[1] = NULL;
+ dummy_options.args = args;
+ dummy_options.exit_cb = fail_cb;
+ dummy_options.flags = 0;
+ for (i = 0; i < 100; i++) {
+ r = uv_spawn(uv_default_loop(), &dummy_processes[i], &dummy_options);
+ if (r != UV_ENOENT)
+ ASSERT_EQ(r, UV_EACCES);
+ uv_close((uv_handle_t*) &dummy_processes[i], close_cb);
+ }
+
+ r = uv_run(uv_default_loop(), UV_RUN_DEFAULT);
+ ASSERT_EQ(r, 0);
+
+ ASSERT_EQ(exit_cb_called, 1);
+ ASSERT_EQ(close_cb_called, 101);
+
+ MAKE_VALGRIND_HAPPY();
+ return 0;
+}
+
/* Helper for child process of spawn_inherit_streams */
#ifndef _WIN32
void spawn_stdin_stdout(void) {
@@ -0,0 +1,54 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash@gmail.com>
Date: Sat, 5 Mar 2022 12:52:04 -0500
Subject: process: only use F_DUPFD_CLOEXEC if it is defined (#3512)

We can save a syscall on most modern systems (required by POSIX 2008),
but not on all systems.

Also handle errors from CLOEXEC. Even though fcntl does not really
define there to be any, it could theoretically be EBADF if the user
happened to pass a bad file descriptor to the same number fd (such that
no other code happened to already fail on that).

diff --git a/deps/uv/src/unix/process.c b/deps/uv/src/unix/process.c
index d208f99be40df9f36447552daf2772c1cab1ce79..7705068730cb0536998bad7d304cb87df99b72e8 100644
--- a/deps/uv/src/unix/process.c
+++ b/deps/uv/src/unix/process.c
@@ -275,9 +275,20 @@ static void uv__process_child_init(const uv_process_options_t* options,
use_fd = pipes[fd][1];
if (use_fd < 0 || use_fd >= fd)
continue;
+#ifdef F_DUPFD_CLOEXEC /* POSIX 2008 */
pipes[fd][1] = fcntl(use_fd, F_DUPFD_CLOEXEC, stdio_count);
+#else
+ pipes[fd][1] = fcntl(use_fd, F_DUPFD, stdio_count);
+#endif
if (pipes[fd][1] == -1)
uv__write_errno(error_fd);
+#ifndef F_DUPFD_CLOEXEC /* POSIX 2008 */
+ n = uv__cloexec_fcntl(pipes[fd][1], 1);
+ if (n) {
+ uv__write_int(error_fd, n);
+ _exit(127);
+ }
+#endif
}

for (fd = 0; fd < stdio_count; fd++) {
@@ -300,8 +311,13 @@ static void uv__process_child_init(const uv_process_options_t* options,
}

if (fd == use_fd) {
- if (close_fd == -1)
- uv__cloexec_fcntl(use_fd, 0);
+ if (close_fd == -1) {
+ n = uv__cloexec_fcntl(use_fd, 0);
+ if (n) {
+ uv__write_int(error_fd, n);
+ _exit(127);
+ }
+ }
}
else {
fd = dup2(use_fd, fd);
@@ -0,0 +1,36 @@
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Jameson Nash <vtjnash@gmail.com>
Date: Fri, 11 Mar 2022 12:05:24 -0500
Subject: process: reset the signal mask if the fork fails (#3537)

Fix a regression that sneaked into posix spawn changes.

Refs: https://github.com/libuv/libuv/pull/3257

diff --git a/deps/uv/src/unix/process.c b/deps/uv/src/unix/process.c
index b85aa3b94edd040952e0d350a47a38d9ba8a67d3..d208f99be40df9f36447552daf2772c1cab1ce79 100644
--- a/deps/uv/src/unix/process.c
+++ b/deps/uv/src/unix/process.c
@@ -790,11 +790,6 @@ static int uv__spawn_and_init_child_fork(const uv_process_options_t* options,

*pid = fork();

- if (*pid == -1) {
- /* Failed to fork */
- return UV__ERR(errno);
- }
-
if (*pid == 0) {
/* Fork succeeded, in the child process */
uv__process_child_init(options, stdio_count, pipes, error_fd);
@@ -804,6 +799,10 @@ static int uv__spawn_and_init_child_fork(const uv_process_options_t* options,
if (pthread_sigmask(SIG_SETMASK, &sigoldset, NULL) != 0)
abort();

+ if (*pid == -1)
+ /* Failed to fork */
+ return UV__ERR(errno);
+
/* Fork succeeded, in the parent process */
return 0;
}

0 comments on commit 322fe50

Please sign in to comment.