[Patch] Rewrite PosixProcess

[David Daney <ddaney at avtrex dot com>]

The bits in PosixProcess were getting stale and I thought it was time to 
freshen them up a some.

The main motivation for the patch is to make it possible for libgcj to 
interact with other code that forks processes and catches SIGCHLD.

The main change is that we iterate through each known PosixProcess and 
try to wait specifically for it instead of all processes whenever we 
receive a SIGCHLD.  This allows other code to wait for its own processes.

Tested on x86_64-pc-linux-gnu (FC6) with no failures in libjava.

OK to commit?

2007-05-06  David Daney  <ddaney@avtrex.com>

   PR libgcj/29324
   * include/posix-threads.h (_Jv_BlockSigchld): Declare.
   (_Jv_UnBlockSigchld): Same.
   * posix-threads.cc: Include posix-threads.h.
   (block_sigchld) Rename to...
   (_Jv_BlockSigchld) ... this.
   (_Jv_UnBlockSigchld): New function.
   (_Jv_InitThreads): Call _Jv_BlockSigchld in place of block_sigchld.
   (_Jv_ThreadStart): Same.
   * java/lang/PosixProcess$ProcessManager.h: Regenerate.
   * java/lang/PosixProcess.java: Clean up imports.
   (ProcessManager): Make final.
   (ProcessManager.queue): Genericise.
   (ProcessManager.pidToProcess): Remove.
   (ProcessManager.liveProcesses): New field.
   (ProcessManager.reaperPID): Remove.
   (ProcessManager.nativeData): New field.
   (ProcessManager.removeProcessFromMap): Remove.
   (ProcessManager.addProcessToMap):Remove.
   (ProcessManager.run): Rewritten.
   (ProcessManager.reap): Change method signature,
   (getErrorStream): Correct formatting.
   (getInputStream): Same.
   (spawn): Add process to liveProcesses list.
   (pid): Make package private.
   * java/lang/PosixProcess.h: Regenerate.
   * java/lang/natPosixProcess.cc: Include posix.h and posix-threads.h.
   Add useing namespace java::lang.
   (ProcessManagerInternal): New struct.
   (sigchld_handler): Rewritten.
   (init): Rewritten.
   (waitForSignal): Same.
   (reap): Same.
   (signalReaper): Same.
   (nativeDestroy): Call kill as ::kill.
   (nativeSpawn): Correct formatting.

Index: include/posix-threads.h
===================================================================
--- include/posix-threads.h	(revision 123987)
+++ include/posix-threads.h	(working copy)
@@ -92,6 +92,14 @@ _Jv_GetPlatformThreadID(_Jv_Thread_t *t)
 }
 
 //
+// Signal helpers.
+//
+
+void _Jv_BlockSigchld();
+void _Jv_UnBlockSigchld();
+
+
+//
 // Condition variables.
 //
 
Index: posix-threads.cc
===================================================================
--- posix-threads.cc	(revision 123987)
+++ posix-threads.cc	(working copy)
@@ -14,6 +14,7 @@ details.  */
 #include <config.h>
 
 #include "posix.h"
+#include "posix-threads.h"
 
 // If we're using the Boehm GC, then we need to override some of the
 // thread primitives.  This is fairly gross.
@@ -472,8 +473,8 @@ handle_intr (int)
   // Do nothing.
 }
 
-static void
-block_sigchld()
+void
+_Jv_BlockSigchld()
 {
   sigset_t mask;
   sigemptyset (&mask);
@@ -484,6 +485,17 @@ block_sigchld()
 }
 
 void
+_Jv_UnBlockSigchld()
+{
+  sigset_t mask;
+  sigemptyset (&mask);
+  sigaddset (&mask, SIGCHLD);
+  int c = pthread_sigmask (SIG_UNBLOCK, &mask, NULL);
+  if (c != 0)
+    JvFail (strerror (c));
+}
+
+void
 _Jv_InitThreads (void)
 {
   pthread_key_create (&_Jv_ThreadKey, NULL);
@@ -501,7 +513,7 @@ _Jv_InitThreads (void)
 
   // Block SIGCHLD here to ensure that any non-Java threads inherit the new 
   // signal mask.
-  block_sigchld();
+  _Jv_BlockSigchld();
 
   // Check/set the thread stack size.
   size_t min_ss = 32 * 1024;
@@ -581,7 +593,7 @@ _Jv_ThreadRegister (_Jv_Thread_t *data)
       }
 # endif
   // Block SIGCHLD which is used in natPosixProcess.cc.
-  block_sigchld();
+  _Jv_BlockSigchld();
 }
 
 void
@@ -629,7 +641,7 @@ _Jv_ThreadStart (java::lang::Thread *thr
 
   // Block SIGCHLD which is used in natPosixProcess.cc.
   // The current mask is inherited by the child thread.
-  block_sigchld();
+  _Jv_BlockSigchld();
 
   param.sched_priority = thread->getPriority();
 
Index: java/lang/PosixProcess$ProcessManager.h
===================================================================
--- java/lang/PosixProcess$ProcessManager.h	(revision 123987)
+++ java/lang/PosixProcess$ProcessManager.h	(working copy)
@@ -7,31 +7,38 @@
 #pragma interface
 
 #include <java/lang/Thread.h>
+extern "Java"
+{
+  namespace gnu
+  {
+    namespace gcj
+    {
+        class RawDataManaged;
+    }
+  }
+}
 
 class java::lang::PosixProcess$ProcessManager : public ::java::lang::Thread
 {
 
 public: // actually package-private
   PosixProcess$ProcessManager();
-private:
-  ::java::lang::PosixProcess * removeProcessFromMap(jlong);
-public: // actually package-private
-  virtual void addProcessToMap(::java::lang::PosixProcess *);
-  virtual void startExecuting(::java::lang::PosixProcess *);
-  virtual void waitUntilReady();
+  void startExecuting(::java::lang::PosixProcess *);
+  void waitUntilReady();
 public:
-  virtual void run();
+  void run();
 private:
   void init();
   void waitForSignal();
-  jboolean reap();
+  jboolean reap(::java::lang::PosixProcess *);
   void signalReaper();
 public: // actually package-private
-  ::java::util::List * __attribute__((aligned(__alignof__( ::java::lang::Thread)))) queue;
+  ::java::util::LinkedList * __attribute__((aligned(__alignof__( ::java::lang::Thread)))) queue;
+  ::java::util::LinkedList * liveProcesses;
 private:
-  ::java::util::Map * pidToProcess;
   jboolean ready;
-  jlong reaperPID;
+public: // actually package-private
+  static ::gnu::gcj::RawDataManaged * nativeData;
 public:
   static ::java::lang::Class class$;
 };
Index: java/lang/PosixProcess.java
===================================================================
--- java/lang/PosixProcess.java	(revision 123987)
+++ java/lang/PosixProcess.java	(working copy)
@@ -13,11 +13,10 @@ import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
-import java.util.HashMap;
+import java.util.Iterator;
 import java.util.LinkedList;
-import java.util.List;
-import java.util.Map;
 
+import gnu.gcj.RawDataManaged;
 
 /**
  * @author Tom Tromey <tromey@cygnus.com>
@@ -27,7 +26,7 @@ import java.util.Map;
  */
 final class PosixProcess extends Process
 {
-  static class ProcessManager extends Thread
+  static final class ProcessManager extends Thread
   {
     /**
      * A list of {@link PosixProcess PosixProcesses} to be
@@ -35,10 +34,11 @@ final class PosixProcess extends Process
      * for all process related operations. To avoid dead lock
      * ensure queueLock is obtained before PosixProcess.
      */
-    List queue = new LinkedList();
-    private Map pidToProcess = new HashMap();
+    LinkedList<PosixProcess> queue = new LinkedList<PosixProcess>();
+    LinkedList<PosixProcess> liveProcesses = new LinkedList<PosixProcess>();
     private boolean ready = false;
-    private long reaperPID;
+
+    static RawDataManaged nativeData;
 
     ProcessManager()
     {
@@ -53,30 +53,6 @@ final class PosixProcess extends Process
     }
 
     /**
-     * Get the PosixProcess object with the given pid and
-     * remove it from the map.  This method is called from the
-     * native code for {@link #reap()).  The mapping is removed so
-     * the PosixProcesses can be GCed after they terminate.
-     *
-     * @param p The pid of the process.
-     */
-    private PosixProcess removeProcessFromMap(long p)
-    {
-      return (PosixProcess) pidToProcess.remove(new Long(p));
-    }
-
-    /**
-     * Put the given PosixProcess in the map using the Long
-     * value of its pid as the key.
-     *
-     * @param p The PosixProcess.
-     */
-    void addProcessToMap(PosixProcess p)
-    {
-      pidToProcess.put(new Long(p.pid), p);
-    }
-
-    /**
      * Queue up the PosixProcess and awake the ProcessManager.
      * The ProcessManager will start the PosixProcess from its
      * thread so it can be reaped when it terminates.
@@ -122,61 +98,66 @@ final class PosixProcess extends Process
       // Now ready to accept requests.
       synchronized (this)
         {
-	  ready = true;
-	  this.notifyAll();
+          ready = true;
+          this.notifyAll();
         }
 
       for (;;)
         {
-	  try
-	    {
-	      synchronized (queueLock)
-	        {
-		  boolean haveMoreChildren = reap();
-		  if (! haveMoreChildren && queue.size() == 0)
-		    {
-		      // This reaper thread could exit, but we
-		      // keep it alive for a while in case
-		      // someone wants to start more Processes.
-		      try
-		        {
-			  queueLock.wait(1000L);
-			  if (queue.size() == 0)
-			    {
-			      processManager = null;
-			      return; // Timed out.
-			    }
-		        }
-		      catch (InterruptedException ie)
-		        {
-			  // Ignore and exit the thread.
-			  return;
-		        }
-		    }
-		  while (queue.size() > 0)
-		    {
-		      PosixProcess p = (PosixProcess) queue.remove(0);
-		      p.spawn(this);
-		    }
-	        }
-
-	      // Wait for a SIGCHLD from either an exiting
-	      // process or the startExecuting() method.  This
-	      // is done outside of the synchronized block to
-	      // allow other threads to enter and submit more
-	      // jobs.
-	      waitForSignal();
-	    }
-	  catch (Exception ex)
-	    {
-	      ex.printStackTrace(System.err);
-	    }
+          try
+            {
+              synchronized (queueLock)
+                {
+                  Iterator<PosixProcess> processIterator =
+                    liveProcesses.iterator();
+                  while (processIterator.hasNext())
+                    {
+                      boolean reaped = reap(processIterator.next());
+                      if (reaped)
+                        processIterator.remove();
+                    }
+                  if (liveProcesses.size() == 0 && queue.size() == 0)
+                    {
+                      // This reaper thread could exit, but we keep it
+                      // alive for a while in case someone wants to
+                      // start more Processes.
+                      try
+                        {
+                          queueLock.wait(1000L);
+                          if (queue.size() == 0)
+                            {
+                              processManager = null;
+                              return; // Timed out.
+                            }
+                        }
+                      catch (InterruptedException ie)
+                        {
+                          // Ignore and exit the thread.
+                          return;
+                        }
+                    }
+                  while (queue.size() > 0)
+                    {
+                      PosixProcess p = queue.remove(0);
+                      p.spawn(this);
+                    }
+                }
+
+              // Wait for a SIGCHLD from either an exiting process or
+              // the startExecuting() method.  This is done outside of
+              // the synchronized block to allow other threads to
+              // enter and submit more jobs.
+              waitForSignal();
+            }
+          catch (Exception ex)
+            {
+              ex.printStackTrace(System.err);
+            }
         }
     }
 
     /**
      * Setup native signal handlers and other housekeeping things.
-     *
      */
     private native void init();
 
@@ -187,12 +168,14 @@ final class PosixProcess extends Process
     private native void waitForSignal();
 
     /**
-     * Try to reap as many children as possible without blocking.
+     * Try to reap the specified child without blocking.
+     *
+     * @param p the process to try to reap.
      *
-     * @return true if more live children exist.
+     * @return true if the process terminated.
      *
      */
-    private native boolean reap();
+    private native boolean reap(PosixProcess p);
 
     /**
      * Send SIGCHLD to the reaper thread.
@@ -295,7 +278,7 @@ final class PosixProcess extends Process
       returnedErrorStream = EOFInputStream.instance;
     else
       returnedErrorStream = errorStream;
-            
+
     return returnedErrorStream;
   }
 
@@ -308,7 +291,7 @@ final class PosixProcess extends Process
       returnedInputStream = EOFInputStream.instance;
     else
       returnedInputStream = inputStream;
-            
+
     return returnedInputStream;
   }
 
@@ -342,7 +325,7 @@ final class PosixProcess extends Process
 	// There is no race with reap() in the pidToProcess map
 	// because this is always called from the same thread
 	// doing the reaping.
-	pm.addProcessToMap(this);
+	pm.liveProcesses.add(this);
 	state = STATE_RUNNING;
 	// Notify anybody waiting on state change.
 	this.notifyAll();
@@ -426,7 +409,7 @@ final class PosixProcess extends Process
   private Throwable exception;
 
   /** The process id.  This is cast to a pid_t on the native side. */
-  private long pid;
+  long pid;
 
   // FIXME: Why doesn't the friend declaration in PosixProcess.h
   // allow PosixProcess$ProcessManager native code access these
Index: java/lang/PosixProcess.h
===================================================================
--- java/lang/PosixProcess.h	(revision 123987)
+++ java/lang/PosixProcess.h	(working copy)
@@ -32,17 +32,16 @@ private:
   void nativeSpawn();
 public: // actually package-private
   PosixProcess(JArray< ::java::lang::String * > *, JArray< ::java::lang::String * > *, ::java::io::File *, jboolean);
-  static jlong access$0(::java::lang::PosixProcess *);
-  static ::java::lang::Object * access$1();
-  static void access$2(::java::lang::PosixProcess$ProcessManager *);
+  static ::java::lang::Object * access$0();
+  static void access$1(::java::lang::PosixProcess$ProcessManager *);
 private:
   JArray< ::java::lang::String * > * __attribute__((aligned(__alignof__( ::java::lang::Process)))) progarray;
   JArray< ::java::lang::String * > * envp;
   ::java::io::File * dir;
   jboolean redirect;
   ::java::lang::Throwable * exception;
-  jlong pid;
 public: // actually package-private
+  jlong pid;
   static const jint STATE_WAITING_TO_START = 0;
   static const jint STATE_RUNNING = 1;
   static const jint STATE_TERMINATED = 2;
Index: java/lang/natPosixProcess.cc
===================================================================
--- java/lang/natPosixProcess.cc	(revision 123987)
+++ java/lang/natPosixProcess.cc	(working copy)
@@ -30,6 +30,8 @@ details.  */
 
 #include <gcj/cni.h>
 #include <jvm.h>
+#include <posix.h>
+#include <posix-threads.h>
 
 #include <java/lang/PosixProcess$ProcessManager.h>
 #include <java/lang/PosixProcess.h>
@@ -48,6 +50,7 @@ details.  */
 #include <java/lang/PosixProcess$EOFInputStream.h>
 
 using gnu::java::nio::channels::FileChannelImpl;
+using namespace java::lang;
 
 extern char **environ;
 
@@ -91,13 +94,37 @@ myclose (int &fd)
   fd = -1;
 }
 
+namespace
+{
+  struct ProcessManagerInternal
+  {
+    int pipe_ends[2];
+    struct sigaction old_sigaction;
+  };
+}
+
+
 // There has to be a signal handler in order to be able to
 // sigwait() on SIGCHLD.  The information passed is ignored as it
 // will be recovered by the waitpid() call.
 static void
-sigchld_handler (int)
+sigchld_handler (int sig, siginfo_t *si, void *third)
 {
-  // Ignore.
+  if (PosixProcess$ProcessManager::nativeData != NULL)
+    {
+      ProcessManagerInternal *pmi =
+        (ProcessManagerInternal *)PosixProcess$ProcessManager::nativeData;
+      char c = 0;
+      ::write(pmi->pipe_ends[1], &c, 1);
+      if (pmi->old_sigaction.sa_handler != SIG_DFL
+          && pmi->old_sigaction.sa_handler != SIG_IGN)
+        {
+          if ((pmi->old_sigaction.sa_flags & SA_SIGINFO) != 0)
+            pmi->old_sigaction.sa_sigaction(sig, si, third);
+          else
+            (*pmi->old_sigaction.sa_handler)(sig);
+        }
+    }
 }
 
 
@@ -105,22 +132,35 @@ sigchld_handler (int)
 void
 java::lang::PosixProcess$ProcessManager::init ()
 {
-  using namespace java::lang;
-  // Remenber our PID so other threads can kill us.
-  reaperPID = (jlong) pthread_self ();
-
-  // SIGCHLD is blocked in all threads in posix-threads.cc.
-  // Setup the SIGCHLD handler.
-  struct sigaction sa;
-  memset (&sa, 0, sizeof (sa));
-
-  sa.sa_handler = sigchld_handler;
-  // We only want signals when the things exit.
-  sa.sa_flags = SA_NOCLDSTOP;
+  // The nativeData is static to avoid races installing the signal
+  // handler in the case that it is chained.
+  if (nativeData == NULL )
+    {
+      ProcessManagerInternal *pmi =
+        (ProcessManagerInternal *)JvAllocBytes(sizeof(ProcessManagerInternal));
 
-  if (-1 == sigaction (SIGCHLD, &sa, NULL))
-    goto error;
+      if (0 != ::pipe(pmi->pipe_ends))
+        goto error;
 
+      // Make writing non-blocking so that the signal handler will
+      // never block.
+      int fl = ::fcntl(pmi->pipe_ends[1], F_GETFL);
+      ::fcntl(pmi->pipe_ends[1], F_SETFL, fl | O_NONBLOCK);
+
+      nativeData = (::gnu::gcj::RawDataManaged *)pmi;
+
+      // SIGCHLD is blocked in all threads in posix-threads.cc.
+      // Setup the SIGCHLD handler.
+      struct sigaction sa;
+      memset (&sa, 0, sizeof (sa));
+
+      sa.sa_sigaction = sigchld_handler;
+      // We only want signals when the things exit.
+      sa.sa_flags = SA_NOCLDSTOP | SA_SIGINFO;
+
+      if (-1 == sigaction (SIGCHLD, &sa, &pmi->old_sigaction))
+        goto error;
+    }
   // All OK.
   return;
 
@@ -132,79 +172,52 @@ void
 java::lang::PosixProcess$ProcessManager::waitForSignal ()
 {
   // Wait for SIGCHLD
-  sigset_t mask;
-  pthread_sigmask (0, NULL, &mask);
-  sigdelset (&mask, SIGCHLD);
-
-  // Use sigsuspend() instead of sigwait() as sigwait() doesn't play
-  // nicely with the GC's use of signals.
-  sigsuspend (&mask);
-
-  // Do not check sigsuspend return value.  The only legitimate return
-  // is EINTR, but there is a known kernel bug affecting alpha-linux
-  // wrt sigsuspend+handler+sigreturn that can result in a return value
-  // of __NR_sigsuspend and errno unset.  Don't fail unnecessarily on
-  // older kernel versions.
+  _Jv_UnBlockSigchld();
+  ProcessManagerInternal *pmi = (ProcessManagerInternal *)nativeData;
+
+  // Try to read multiple (64) notifications in one go.
+  char c[64];
+ ::read(pmi->pipe_ends[0], c, sizeof (c));
+
+  _Jv_BlockSigchld();
 
-  // All OK.
   return;
 }
 
-jboolean java::lang::PosixProcess$ProcessManager::reap ()
+jboolean java::lang::PosixProcess$ProcessManager::reap (PosixProcess *p)
 {
-  using namespace java::lang;
-
-  pid_t pid;
-
-  for (;;)
-    {
-      // Get the return code from a dead child process.
-      int status;
-      pid = waitpid ((pid_t) - 1, &status, WNOHANG);
-      if (pid == -1)
-	{
-	  if (errno == ECHILD)
-	    return false;
-	  else
-	    goto error;
-	}
+  pid_t rv;
 
-      if (pid == 0)
-        return true;   // No children to wait for.
-
-      // Look up the process in our pid map.
-      PosixProcess * process = removeProcessFromMap ((jlong) pid);
-
-      // Note that if process==NULL, then we have an unknown child.
-      // This is not common, but can happen, and isn't an error.
-      if (process)
-	{
-	  JvSynchronize sync (process);
-	  process->status = WIFEXITED (status) ? WEXITSTATUS (status) : -1;
-	  process->state = PosixProcess::STATE_TERMINATED;
-          process->processTerminationCleanup();
-	  process->notifyAll ();
-	}
-    }
-
-error:
-  throw new InternalError (JvNewStringUTF (strerror (errno)));
+  // Try to get the return code from the child process.
+  int status;
+  rv = ::waitpid ((pid_t)p->pid, &status, WNOHANG);
+  if (rv == -1)
+    throw new InternalError (JvNewStringUTF (strerror (errno)));
+
+  if (rv == 0)
+    return false;   // No children to wait for.
+
+  JvSynchronize sync (p);
+  p->status = WIFEXITED (status) ? WEXITSTATUS (status) : -1;
+  p->state = PosixProcess::STATE_TERMINATED;
+  p->processTerminationCleanup();
+  p->notifyAll ();
+  return true;
 }
 
 void
 java::lang::PosixProcess$ProcessManager::signalReaper ()
 {
-  int c = pthread_kill ((pthread_t) reaperPID, SIGCHLD);
-  if (c == 0)
-    return;
-  // pthread_kill() failed.
-  throw new InternalError (JvNewStringUTF (strerror (c)));
+  ProcessManagerInternal *pmi = (ProcessManagerInternal *)nativeData;
+  char c = 0;
+  ::write(pmi->pipe_ends[1], &c, 1);
+  // Ignore errors.  If EPIPE the reaper has already exited.
 }
 
 void
 java::lang::PosixProcess::nativeDestroy ()
 {
-  int c = kill ((pid_t) pid, SIGKILL);
+  int c = ::kill ((pid_t) pid, SIGKILL);
   if (c == 0)
     return;
   // kill() failed.
@@ -427,9 +440,9 @@ java::lang::PosixProcess::nativeSpawn ()
       char c;
       int r = read (msgp[0], &c, 1);
       if (r == -1)
-      throw new IOException (JvNewStringUTF (strerror (errno)));
+        throw new IOException (JvNewStringUTF (strerror (errno)));
       else if (r != 0)
-      throw new IOException (JvNewStringUTF (strerror (c)));
+        throw new IOException (JvNewStringUTF (strerror (c)));
     }
   catch (java::lang::Throwable *thrown)
     {