fix subprocess checking when the process moves itself to a new group

Closes PR 11200

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 
+/* When the Racket process value for a process in a different group becomes 
-   before a waitpid() on the process, then we need to keep
+   GC-unreachable before a waitpid() on the process, then we 
-   waiting on it to let the OS gc the process. Otherwise,
+   need to keep waiting on the pid to let the OS gc the process.
-   we wait only on processes in the same group as Racket. 
+   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) {
+  /* Check specific pid, in case the child has its own group
-    /* need to specifically try the pid, since we don't
+     (either given by Racket or given to itself): */
-       wait on other process groups in the background thread */
+  {
     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;
+    unused_status = unused_pid_statuses;
-    prev_group = NULL;
+    prev_unused = NULL;
 
     do {
-      if (group_status) {
+      if (unused_status) {
-        check_pid = group_status->pid;
+        /* 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;
+            prev_unused = unused_status;
-            group_status = group_status->next;
+            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;
+          next = unused_status->next;
-          if (prev_group)
+          if (prev_unused)
-            prev_group->next_group = next;
+            prev_unused->next_unused = next;
           else
-            child_group_statuses = next;
+            unused_pid_statuses = next;
-          free(group_status);
+          free(unused_status);
-          group_status = next;
+          unused_status = next;
         } else
           add_child_status(pid, status);
       } else {
         if (is_group) {
-          prev_group = group_status;
+          prev_unused = unused_status;
-          group_status = group_status->next;
+          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) {
+        if (keep_unused) {
-          st->next_group = child_group_statuses;
+          st->next_unused = unused_pid_statuses;
-          child_group_statuses = st;
+          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);
   }

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;
+  if (!((System_Child *)sp->handle)->done) {
-  unused_group = malloc(sizeof(void *) * 2);
+    void **unused_pid;
-  unused_group[0] = (void *)(intptr_t)sp->pid;
+    unused_pid = malloc(sizeof(void *) * 2);
-  unused_group[1] = unused_groups;
+    unused_pid[0] = (void *)(intptr_t)sp->pid;
-  need_to_check_children = 1;
+    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_record, NULL);
-    scheme_add_finalizer(subproc, unused_process_group, NULL);
 # endif
 
   if (SCHEME_TRUEP(cust_mode)) {