Fix the interaction of signals in GAP and the IO package

src/iostream.c

@@ -242,6 +242,31 @@ static UInt OpenPty(int * master, int * slave)
 **  Start a subprocess using ptys. Returns the stream number of the IOStream
 **  that is connected to the new processs
 */
+
+
+// Clean up a signalled or exited child process
+// CheckChildStatusChanged must be called by libraries which replace GAP's
+// signal handler, or call 'waitpid'.
+// The function should be passed a PID, and the return value of waitpid.
+// Returns 1 if that PID was a child owned by GAP, or 0 otherwise.
+int CheckChildStatusChanged(int childPID, int status)
+{
+    GAP_ASSERT(childPID > 0);
+    GAP_ASSERT((WIFEXITED(status) || WIFSIGNALED(status)));
+    HashLock(PtyIOStreams);
+    for (UInt i = 0; i < MAX_PTYS; i++) {
+        if (PtyIOStreams[i].inuse && PtyIOStreams[i].childPID == childPID) {
+            PtyIOStreams[i].changed = 1;
+            PtyIOStreams[i].status = status;
+            PtyIOStreams[i].blocked = 0;
+            HashUnlock(PtyIOStreams);
+            return 1;
+        }
+    }
+    HashUnlock(PtyIOStreams);
+    return 0;
+}
+
 static void ChildStatusChanged(int whichsig)
 {
     UInt i;
@@ -593,14 +618,14 @@ Obj FuncCLOSE_PTY_IOSTREAM(Obj self, Obj stream)
 {
     UInt pty = HashLockStreamIfAvailable(stream);
 
-    PtyIOStreams[pty].inuse = 0;
-
     /* Close down the child */
     int status;
     int retcode = close(PtyIOStreams[pty].ptyFD);
     if (retcode)
         Pr("Strange close return code %d\n", retcode, 0);
     kill(PtyIOStreams[pty].childPID, SIGTERM);
+    // GAP (or another library) might wait on this PID before
+    // we handle it. If that happens, waitpid will return -1.
     retcode = waitpid(PtyIOStreams[pty].childPID, &status, WNOHANG);
     if (retcode == 0) {
         // Give process a second to quit
@@ -612,6 +637,9 @@ Obj FuncCLOSE_PTY_IOSTREAM(Obj self, Obj stream)
         kill(PtyIOStreams[pty].childPID, SIGKILL);
         retcode = waitpid(PtyIOStreams[pty].childPID, &status, 0);
     }
+
+    PtyIOStreams[pty].inuse = 0;
+
     FreeStream(pty);
     HashUnlock(PtyIOStreams);
     return 0;

src/iostream.h

@@ -13,7 +13,11 @@
 #ifndef GAP_IOSTREAM_H
 #define GAP_IOSTREAM_H
 
+// Provide a feature macro to let libraries check if GAP supports
+// CheckChildStatusChanged.
+#define GAP_HasCheckChildStatusChanged
 
+int CheckChildStatusChanged(int childPID, int status);
 
 /*F * * * * * * * * * * * * * initialize package * * * * * * * * * * * * * * *
 */

tst/teststandard/processes/check.pl

@@ -0,0 +1,12 @@
+#!/usr/bin/env perl
+use strict;
+use warnings;
+
+use Time::HiRes;
+
+if ( $ARGV[1] == 1 ) {
+	$SIG{INT} = 'IGNORE';
+	$SIG{TERM} = 'IGNORE';
+}
+
+Time::HiRes::sleep($ARGV[0]/1000)

tst/teststandard/processes/children.tst

@@ -0,0 +1,18 @@
+gap> d := DirectoryCurrent();;
+gap> scriptdir := DirectoriesLibrary( "tst/teststandard/processes/" );;
+gap> checkpl := Filename(scriptdir, "check.pl");;
+
+# If IO is loaded, disable its signal handler
+gap> if IsBoundGlobal("IO_RestoreSIGCHLDHandler") then
+> ValueGlobal("IO_RestoreSIGCHLDHandler")();
+> fi;
+gap> runChild := function(ms, ignoresignals)
+>    local signal;
+>    if ignoresignals then signal := "1"; else signal := "0"; fi;
+>    return InputOutputLocalProcess(d, checkpl, [ String(time), signal]);
+>  end;;
+gap> for i in [1..200] do
+> children := List([1..20], x -> runChild(Random([1..2000]), Random([false,true])));;
+> if ForAny(children, x -> x=fail) then Print("Failed producing child\n"); fi;
+> Perform(children, CloseStream);
+> od;