From ea51d32e9d30deadeb80d7c97614827dd8a35f0a Mon Sep 17 00:00:00 2001 From: Matthew Flatt Date: Sat, 8 Jan 2011 09:48:08 -0700 Subject: [PATCH] fix subprocess checking when the process moves itself to a new group Closes PR 11200 --- src/racket/src/places.c | 70 +++++++++++++++++++++++------------------ src/racket/src/port.c | 44 +++++++++++++++----------- 2 files changed, 64 insertions(+), 50 deletions(-) diff --git a/src/racket/src/places.c b/src/racket/src/places.c index 9c51ef6b96..07deb8f6f0 100644 --- a/src/racket/src/places.c +++ b/src/racket/src/places.c @@ -234,19 +234,22 @@ typedef struct Child_Status { char is_group; void *signal_fd; struct Child_Status *next; - struct Child_Status *next_group; /* see child_group_statuses */ + struct Child_Status *next_unused; /* see unused_pid_statuses */ } Child_Status; SHARED_OK static Child_Status *child_statuses = NULL; SHARED_OK static mzrt_mutex* child_status_lock = NULL; SHARED_OK static mzrt_mutex* child_wait_lock = NULL; /* ordered before status lock */ -/* When a process for a process group becomes unreachable - before a waitpid() on the process, then we need to keep - waiting on it to let the OS gc the process. Otherwise, - we wait only on processes in the same group as Racket. +/* When the Racket process value for a process in a different group becomes + GC-unreachable before a waitpid() on the process, then we + need to keep waiting on the pid to let the OS gc the process. + This list is especially needed for processes that we create in + their own group, but it's also needed for processes that put + themselves in their own group (which we conservatively assume + can be any child process). This list is protect by the wait lock. */ -SHARED_OK static Child_Status *child_group_statuses = NULL; +SHARED_OK static Child_Status *unused_pid_statuses = NULL; static void add_group_signal_fd(void *signal_fd); static void remove_group_signal_fd(void *signal_fd); @@ -270,7 +273,7 @@ static void add_child_status(int pid, int status) { st->signal_fd = NULL; st->next = child_statuses; child_statuses = st; - st->next_group = NULL; + st->next_unused = NULL; st->unneeded = 0; st->is_group = 0; } @@ -317,9 +320,9 @@ static int raw_get_child_status(int pid, int *status, int done_only, int do_remo int scheme_get_child_status(int pid, int is_group, int *status) { int found = 0; - if (is_group) { - /* need to specifically try the pid, since we don't - wait on other process groups in the background thread */ + /* Check specific pid, in case the child has its own group + (either given by Racket or given to itself): */ + { pid_t pid2; int status; @@ -361,7 +364,7 @@ int scheme_places_register_child(int pid, int is_group, void *signal_fd, int *st st->next = child_statuses; child_statuses = st; - st->next_group = NULL; + st->next_unused = NULL; if (is_group) add_group_signal_fd(signal_fd); @@ -375,7 +378,7 @@ static void *mz_proc_thread_signal_worker(void *data) { int status; int pid, check_pid, is_group; sigset_t set; - Child_Status *group_status, *prev_group, *next; + Child_Status *unused_status, *prev_unused, *next; sigemptyset(&set); sigaddset(&set, SIGCHLD); @@ -399,14 +402,18 @@ static void *mz_proc_thread_signal_worker(void *data) { mzrt_mutex_lock(child_wait_lock); - group_status = child_group_statuses; - prev_group = NULL; + unused_status = unused_pid_statuses; + prev_unused = NULL; do { - if (group_status) { - check_pid = group_status->pid; + if (unused_status) { + /* See unused_pid_statuses above */ + check_pid = unused_status->pid; is_group = 1; } else { + /* We wait only on processes in the same group as Racket, + because detecting the termination of a group's main process + disables our ability to terminate all processes in the group. */ check_pid = 0; /* => processes in the same group as Racket */ is_group = 0; } @@ -423,26 +430,26 @@ static void *mz_proc_thread_signal_worker(void *data) { fprintf(stderr, "unexpected error from waitpid(%d[%d]): %d\n", check_pid, is_group, errno); if (is_group) { - prev_group = group_status; - group_status = group_status->next; + prev_unused = unused_status; + unused_status = unused_status->next; } } } else if (pid > 0) { /* printf("SIGCHILD pid %i with status %i %i\n", pid, status, WEXITSTATUS(status)); */ if (is_group) { - next = group_status->next; - if (prev_group) - prev_group->next_group = next; + next = unused_status->next; + if (prev_unused) + prev_unused->next_unused = next; else - child_group_statuses = next; - free(group_status); - group_status = next; + unused_pid_statuses = next; + free(unused_status); + unused_status = next; } else add_child_status(pid, status); } else { if (is_group) { - prev_group = group_status; - group_status = group_status->next; + prev_unused = unused_status; + unused_status = unused_status->next; } } } while ((pid > 0) || is_group); @@ -456,16 +463,17 @@ static void *mz_proc_thread_signal_worker(void *data) { void scheme_done_with_process_id(int pid, int is_group) { Child_Status *st; + int keep_unused = 1; /* assume that any process can be in a new group */ - mzrt_mutex_lock(child_wait_lock); /* protects child_group_statuses */ + mzrt_mutex_lock(child_wait_lock); /* protects unused_pid_statuses */ mzrt_mutex_lock(child_status_lock); for (st = child_statuses; st; st = st->next) { if (st->pid == pid) { if (!st->done) { - if (is_group) { - st->next_group = child_group_statuses; - child_group_statuses = st; + if (keep_unused) { + st->next_unused = unused_pid_statuses; + unused_pid_statuses = st; if (st->signal_fd) remove_group_signal_fd(st->signal_fd); } else @@ -476,7 +484,7 @@ void scheme_done_with_process_id(int pid, int is_group) } } - if (st && (is_group || st->done)) { + if (st && (keep_unused || st->done)) { /* remove it from normal list: */ raw_get_child_status(pid, NULL, 0, 1, !st->done); } diff --git a/src/racket/src/port.c b/src/racket/src/port.c index 1b6612e1ef..621f6a5bdb 100644 --- a/src/racket/src/port.c +++ b/src/racket/src/port.c @@ -6947,7 +6947,10 @@ static int MyPipe(int *ph, int near_index) { # define GC_write_barrier(x) /* empty */ #endif -SHARED_OK static void *unused_groups; +/* See `unused_pid_statuses' in "places.c" for + a reminder of why this is needed (in both + implementations): */ +SHARED_OK static void *unused_pids; static int need_to_check_children; @@ -6996,18 +6999,18 @@ static void init_sigchld(void) static void check_child_done(pid_t pid) { pid_t result, check_pid; - int status, is_group; + int status, is_unused; System_Child *sc, *prev; - void **unused = (void **)unused_groups, **unused_prev = NULL; + void **unused = (void **)unused_pids, **unused_prev = NULL; if (scheme_system_children) { do { if (!pid && unused) { check_pid = (pid_t)(intptr_t)unused[0]; - is_group = 1; + is_unused = 1; } else { check_pid = pid; - is_group = 0; + is_unused = 0; } do { @@ -7017,14 +7020,14 @@ static void check_child_done(pid_t pid) } while ((result == -1) && (errno == EINTR)); if (result > 0) { - if (is_group) { + if (is_unused) { /* done with an inaccessible group id */ void *next; next = (void **)unused[1]; if (unused_prev) unused_prev[1] = unused[1]; else - unused_groups = unused[1]; + unused_pids = unused[1]; free(unused); unused = (void **)next; } @@ -7051,12 +7054,12 @@ static void check_child_done(pid_t pid) } } } else { - if (is_group) { + if (is_unused) { unused_prev = unused; unused = unused[1]; } } - } while ((result > 0) || is_group); + } while ((result > 0) || is_unused); } } @@ -7292,7 +7295,9 @@ static int subp_done(Scheme_Object *so) { System_Child *sc; sc = (System_Child *)sp->handle; - check_child_done(sp->is_group ? sp->pid : 0); + /* Check specific pid, in case the child has its own group + (either given by Racket or given to itself): */ + check_child_done(sp->pid); if (sc->done) child_mref_done(sp); return sc->done; @@ -7350,7 +7355,7 @@ static Scheme_Object *subprocess_status(int argc, Scheme_Object **argv) } # else System_Child *sc = (System_Child *)sp->handle; - check_child_done(sp->is_group ? sp->pid : 0); + check_child_done(sp->pid); if (sc->done) { child_mref_done(sp); @@ -7574,7 +7579,7 @@ static Scheme_Object *subproc_group_on (int argc, Scheme_Object *argv[]) } #ifdef UNIX_PROCESSES -static void unused_process_group(void *_sp, void *ignored) +static void unused_process_record(void *_sp, void *ignored) { Scheme_Subprocess *sp = (Scheme_Subprocess *)_sp; @@ -7582,11 +7587,13 @@ static void unused_process_group(void *_sp, void *ignored) if (!sp->done) scheme_done_with_process_id(sp->pid, sp->is_group); # else - void **unused_group; - unused_group = malloc(sizeof(void *) * 2); - unused_group[0] = (void *)(intptr_t)sp->pid; - unused_group[1] = unused_groups; - need_to_check_children = 1; + if (!((System_Child *)sp->handle)->done) { + void **unused_pid; + unused_pid = malloc(sizeof(void *) * 2); + unused_pid[0] = (void *)(intptr_t)sp->pid; + unused_pid[1] = unused_pids; + need_to_check_children = 1; + } # endif } #endif @@ -8254,8 +8261,7 @@ static Scheme_Object *subprocess(int c, Scheme_Object *args[]) # if defined(WINDOWS_PROCESSES) scheme_add_finalizer(subproc, close_subprocess_handle, NULL); # else - if (new_process_group) - scheme_add_finalizer(subproc, unused_process_group, NULL); + scheme_add_finalizer(subproc, unused_process_record, NULL); # endif if (SCHEME_TRUEP(cust_mode)) {