re PR libgcj/11801 (Problems with Process.waitFor() and exitValue())

2004-08-12  David Daney  <ddaney@avtrex.com>

	PR libgcj/11801
	* java/lang/PosixProcess.java: Rewrote.
	* java/lang/natPosixProcess.cc: Rewrote.
	* java/lang/Runtime.java (execInternal): Declare throws IOException.
	* gcj/javaprims.h (ConcreteProcess$ProcessManager): Declare.
	* posix-threads.cc (block_sigchld) New function.
	(_Jv_ThreadRegister) Use it.
	(_Jv_ThreadStart) Use it.
	* configure.in (PLATFORM_INNER_NAT_HDRS): New AC_SUBST() used in...
	* Makefile.am: ... to specify extra native headers.
	* configure: Regenerated.
	* include/config.h: Regenerated.
	* Makefile.in: Regenerated.
	* gcj/Makefile.in: Regenerated.
	* include/Makefile.in: Regenerated.
	* testsuite/Makefile.in: Regenerated.

From-SVN: r85880

libjava/java/lang/PosixProcess.java

@@ -1,6 +1,5 @@
 // PosixProcess.java - Subclass of Process for POSIX systems.
-
-/* Copyright (C) 1998, 1999  Free Software Foundation
+/* Copyright (C) 1998, 1999, 2004  Free Software Foundation
 
    This file is part of libgcj.
 
@@ -11,76 +10,466 @@ details.  */
 package java.lang;
 
 import java.io.File;
+import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
-import java.io.IOException;
+import java.util.HashMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+
 
 /**
  * @author Tom Tromey <tromey@cygnus.com>
  * @date May 3, 1999
+ * @author David Daney <ddaney@avtrex.com> Rewrote using
+ * ProcessManager
  */
 
 // This is entirely internal to our implementation.
-
 // This file is copied to `ConcreteProcess.java' before compilation.
 // Hence the class name apparently does not match the file name.
 final class ConcreteProcess extends Process
 {
-  public native void destroy ();
-
-  public int exitValue ()
+  static class ProcessManager extends Thread
   {
-    if (! hasExited)
-      throw new IllegalThreadStateException("Process has not exited");
+    /**
+     * A list of {@link ConcreteProcess ConcreteProcesses} to be
+     * started.  The queueLock object is used as the lock Object
+     * for all process related operations. To avoid dead lock
+     * ensure queueLock is obtained before ConcreteProcess.
+     */
+    List queue = new LinkedList();
+    private Map pidToProcess = new HashMap();
+    private boolean ready = false;
+    private long reaperPID;
+
+    ProcessManager()
+    {
+      super("ProcessManager");
+      // Don't keep the (main) process from exiting on our account.
+      this.setDaemon(true);
+    }
+
+    /**
+     * Get the ConcreteProcess 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 ConcreteProcesses can be GCed after they terminate.
+     *
+     * @param p The pid of the process.
+     */
+    private ConcreteProcess removeProcessFromMap(long p)
+    {
+      return (ConcreteProcess) pidToProcess.remove(new Long(p));
+    }
+
+    /**
+     * Put the given ConcreteProcess in the map using the Long
+     * value of its pid as the key.
+     *
+     * @param p The ConcreteProcess.
+     */
+    void addProcessToMap(ConcreteProcess p)
+    {
+      pidToProcess.put(new Long(p.pid), p);
+    }
+
+    /**
+     * Queue up the ConcreteProcess and awake the ProcessManager.
+     * The ProcessManager will start the ConcreteProcess from its
+     * thread so it can be reaped when it terminates.
+     *
+     * @param p The ConcreteProcess.
+     */
+    void startExecuting(ConcreteProcess p)
+    {
+      synchronized (queueLock)
+        {
+	  queue.add(p);
+	  signalReaper(); // If blocked in waitForSignal().
+	  queueLock.notifyAll(); // If blocked in wait();
+        }
+    }
+
+    /**
+     * Block until the ProcessManager thread is ready to accept
+     * commands.
+     */
+    void waitUntilReady()
+    {
+      synchronized (this)
+        {
+	  try
+	    {
+	      while (! ready)
+		wait();
+	    }
+	  catch (InterruptedException ie)
+	    {
+	      // Ignore.
+	    }
+        }
+    }
+
+    /**
+     * Main Process starting/reaping loop.
+     */
+    public void run()
+    {
+      init();
+      // Now ready to accept requests.
+      synchronized (this)
+        {
+	  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)
+		    {
+		      ConcreteProcess p = (ConcreteProcess) 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();
+
+    /**
+     * Block waiting for SIGCHLD.
+     *
+     */
+    private native void waitForSignal();
+
+    /**
+     * Try to reap as many children as possible without blocking.
+     *
+     * @return true if more live children exist.
+     *
+     */
+    private native boolean reap();
+
+    /**
+     * Send SIGCHLD to the reaper thread.
+     */
+    private native void signalReaper();
+  }
+
+  public void destroy()
+  {
+    // Synchronized on the queueLock.  This ensures that the reaper
+    // thread cannot be doing a wait() on the child.
+    // Otherwise there would be a race where the OS could
+    // create a process with the same pid between the wait()
+    // and the update of the state which would cause a kill to
+    // the wrong process.
+    synchronized (queueLock)
+      {
+	synchronized (this)
+	  {
+	    // If there is no ProcessManager we cannot kill.
+	    if (state != STATE_TERMINATED)
+	      {
+		if (processManager == null)
+		  throw new InternalError();
+		nativeDestroy();
+	      }
+	  }
+      }
+  }
+
+  private native void nativeDestroy();
+
+  public int exitValue()
+  {
+    synchronized (this)
+      {
+	if (state != STATE_TERMINATED)
+	  throw new IllegalThreadStateException("Process has not exited");
+      }
     return status;
   }
 
-  public InputStream getErrorStream ()
+  /**
+   * Called by native code when process exits.
+   *
+   * Already synchronized (this).  Close any streams that we can to
+   * conserve file descriptors.
+   *
+   * The outputStream can be closed as any future writes will
+   * generate an IOException due to EPIPE.
+   *
+   * The inputStream and errorStream can only be closed if the user
+   * has not obtained a reference to them AND they have no bytes
+   * available.  Since the process has terminated they will never have
+   * any more data available and can safely be replaced by
+   * EOFInputStreams.
+   */
+  void processTerminationCleanup()
   {
-    return errorStream;
+    try
+      {
+        outputStream.close();
+      }
+    catch (IOException ioe)
+      {
+        // Ignore.
+      }
+    try
+      {
+        if (returnedErrorStream == null && errorStream.available() == 0)
+          {
+            errorStream.close();
+            errorStream = null;
+          }
+      }
+    catch (IOException ioe)
+      {
+        // Ignore.
+      }
+    try
+      {
+        if (returnedInputStream == null && inputStream.available() == 0)
+          {
+            inputStream.close();
+            inputStream = null;
+          }
+      }
+    catch (IOException ioe)
+      {
+        // Ignore.
+      }
   }
 
-  public InputStream getInputStream ()
+  public synchronized InputStream getErrorStream()
   {
-    return inputStream;
+    if (returnedErrorStream != null)
+      return returnedErrorStream;
+
+    if (errorStream == null)
+      returnedErrorStream = EOFInputStream.instance;
+    else
+      returnedErrorStream = errorStream;
+            
+    return returnedErrorStream;
   }
 
-  public OutputStream getOutputStream ()
+  public synchronized InputStream getInputStream()
+  {
+    if (returnedInputStream != null)
+      return returnedInputStream;
+
+    if (inputStream == null)
+      returnedInputStream = EOFInputStream.instance;
+    else
+      returnedInputStream = inputStream;
+            
+    return returnedInputStream;
+  }
+
+  public OutputStream getOutputStream()
   {
     return outputStream;
   }
 
-  public native int waitFor () throws InterruptedException;
+  public int waitFor() throws InterruptedException
+  {
+    synchronized (this)
+      {
+	while (state != STATE_TERMINATED)
+	  wait();
+      }
+    return status;
+  }
 
-  // This is used for actual initialization, as we can't write a
-  // native constructor.
-  public native void startProcess (String[] progarray,
-                                   String[] envp,
-                                   File dir)
-    throws IOException;
+  /**
+   * Start this process running.  This should only be called by the
+   * ProcessManager.
+   *
+   * @param pm The ProcessManager that made the call.
+   */
+  void spawn(ProcessManager pm)
+  {
+    synchronized (this)
+      {
+	// Do the fork/exec magic.
+	nativeSpawn();
+	// 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);
+	state = STATE_RUNNING;
+	// Notify anybody waiting on state change.
+	this.notifyAll();
+      }
+  }
+
+  /**
+   * Do the fork and exec.
+   */
+  private native void nativeSpawn();
 
   // This file is copied to `ConcreteProcess.java' before
   // compilation.  Hence the constructor name apparently does not
   // match the file name.
-  public ConcreteProcess (String[] progarray,
-                          String[] envp,
-                          File dir)
-    throws IOException
+  ConcreteProcess(String[] progarray, String[] envp, File dir)
+           throws IOException
   {
-    startProcess (progarray, envp, dir);
+    // Check to ensure there is something to run, and avoid
+    // dereferencing null pointers in native code.
+    if (progarray[0] == null)
+      throw new NullPointerException();
+
+    this.progarray = progarray;
+    this.envp = envp;
+    this.dir = dir;
+
+    // Start a ProcessManager if there is not one already running.
+    synchronized (queueLock)
+      {
+	if (processManager == null)
+	  {
+	    processManager = new ProcessManager();
+	    processManager.start();
+	    processManager.waitUntilReady();
+	  }
+
+	// Queue this ConcreteProcess for starting by the ProcessManager.
+	processManager.startExecuting(this);
+      }
+
+    // Wait until ProcessManager has started us.
+    synchronized (this)
+      {
+	while (state == STATE_WAITING_TO_START)
+	  {
+	    try
+	      {
+		wait();
+	      }
+	    catch (InterruptedException ie)
+	      {
+		// FIXME: What to do when interrupted while blocking in a constructor?
+		// Ignore.
+	      }
+	  }
+      }
+
+    // If there was a problem, re-throw it.
+    if (exception != null)
+      {
+	if (exception instanceof IOException)
+	  {
+	    IOException ioe = new IOException(exception.toString());
+	    ioe.initCause(exception);
+	    throw ioe;
+	  }
+
+	// Not an IOException.  Something bad happened.
+	InternalError ie = new InternalError(exception.toString());
+	ie.initCause(exception);
+	throw ie;
+      }
+
+    // If we get here, all is well, the Process has started.
   }
 
-  // The process id.  This is cast to a pid_t on the native side.
+  private String[] progarray;
+  private String[] envp;
+  private File dir;
+
+  /** Set by the ProcessManager on problems starting. */
+  private Throwable exception;
+
+  /** The process id.  This is cast to a pid_t on the native side. */
   private long pid;
 
-  // True when child has exited.
-  private boolean hasExited;
+  // FIXME: Why doesn't the friend declaration in ConcreteProcess.h
+  // allow ConcreteProcess$ProcessManager native code access these
+  // when they are private?
 
-  // The exit status, if the child has exited.
-  private int status;
+  /** Before the process is forked. */
+  static final int STATE_WAITING_TO_START = 0;
 
-  // The streams.
+  /** After the fork. */
+  static final int STATE_RUNNING = 1;
+
+  /** After exit code has been collected. */
+  static final int STATE_TERMINATED = 2;
+
+  /** One of STATE_WAITING_TO_START, STATE_RUNNING, STATE_TERMINATED. */
+  int state;
+
+  /** The exit status, if the child has exited. */
+  int status;
+
+  /** The streams. */
   private InputStream errorStream;
   private InputStream inputStream;
   private OutputStream outputStream;
+
+  /** InputStreams obtained by the user.  Not null indicates that the
+   *  user has obtained the stream.
+   */
+  private InputStream returnedErrorStream;
+  private InputStream returnedInputStream;
+
+  /**
+   * Lock Object for all processManager related locking.
+   */
+  private static Object queueLock = new Object();
+  private static ProcessManager processManager;
+
+  static class EOFInputStream extends InputStream
+  {
+    static EOFInputStream instance = new EOFInputStream();
+    public int read()
+    {
+      return -1;
+    }
+  }
 }

libjava/java/lang/Runtime.java

@@ -1,5 +1,5 @@
 /* Runtime.java -- access to the VM process
-   Copyright (C) 1998, 2002, 2003 Free Software Foundation
+   Copyright (C) 1998, 2002, 2003, 2004 Free Software Foundation
 
 This file is part of GNU Classpath.
 
@@ -734,8 +734,11 @@ public class Runtime
    * @param dir the directory to use, may be null
    * @return the newly created process
    * @throws NullPointerException if cmd or env have null elements
+   * @throws IOException if the exec fails
    */
-  native Process execInternal(String[] cmd, String[] env, File dir);
+  native Process execInternal(String[] cmd, String[] env, File dir)
+    throws IOException;
+    
 
   /**
    * Get the system properties. This is done here, instead of in System,

libjava/java/lang/natPosixProcess.cc

@@ -21,12 +21,16 @@ details.  */
 #include <string.h>
 #include <stdlib.h>
 #include <stdio.h>
+#include <unistd.h>
+#include <pthread.h>
 
 #include <gcj/cni.h>
 #include <jvm.h>
 
+#include <java/lang/ConcreteProcess$ProcessManager.h>
 #include <java/lang/ConcreteProcess.h>
 #include <java/lang/IllegalThreadStateException.h>
+#include <java/lang/InternalError.h>
 #include <java/lang/InterruptedException.h>
 #include <java/lang/NullPointerException.h>
 #include <java/lang/Thread.h>
@@ -42,44 +46,6 @@ using gnu::java::nio::channels::FileChannelImpl;
 
 extern char **environ;
 
-void
-java::lang::ConcreteProcess::destroy (void)
-{
-  if (! hasExited)
-    {
-      // Really kill it.
-      kill ((pid_t) pid, SIGKILL);
-    }
-}
-
-jint
-java::lang::ConcreteProcess::waitFor (void)
-{
-  if (! hasExited)
-    {
-      int wstat;
-      int r = waitpid ((pid_t) pid, &wstat, 0);
-
-      if (r == -1)
-        {
-	  if (java::lang::Thread::interrupted())
-	    throw new InterruptedException (JvNewStringLatin1 (strerror
-	      (errno)));
-	}
-      else
-	{
-	  hasExited = true;
-
-	  if (WIFEXITED (wstat))
-	    status = WEXITSTATUS (wstat);
-	  else
-	    status = -1;
-	}
-    }
-
-  return status;
-}
-
 static char *
 new_string (jstring string)
 {
@@ -120,15 +86,135 @@ myclose (int &fd)
   fd = -1;
 }
 
+// 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)
+{
+  // Ignore.
+}
+
+
+// Get ready to enter the main reaper thread loop.
 void
-java::lang::ConcreteProcess::startProcess (jstringArray progarray,
-					   jstringArray envp,
-					   java::io::File *dir)
+java::lang::ConcreteProcess$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;
+
+  if (-1 == sigaction (SIGCHLD, &sa, NULL))
+    goto error;
+
+  // All OK.
+  return;
+
+error:
+  throw new InternalError (JvNewStringUTF (strerror (errno)));
+}
+
+void
+java::lang::ConcreteProcess$ProcessManager::waitForSignal ()
+{
+  using namespace java::lang;
+
+  sigset_t mask;
+  // Wait for SIGCHLD
+  sigemptyset (&mask);
+  sigaddset (&mask, SIGCHLD);
+
+  int sig;
+  int c = sigwait (&mask, &sig);
+
+  if (c != 0)
+    goto error;
+
+  // All OK.
+  return;
+
+error:
+  throw new InternalError (JvNewStringUTF (strerror (c)));
+}
+
+jboolean java::lang::ConcreteProcess$ProcessManager::reap ()
+{
+  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;
+	}
+
+      if (pid == 0)
+        return true;   // No children to wait for.
+
+      // Look up the process in our pid map.
+      ConcreteProcess * process = removeProcessFromMap ((jlong) pid);
+
+      if (process)
+	{
+	  JvSynchronize sync (process);
+	  process->status = WIFEXITED (status) ? WEXITSTATUS (status) : -1;
+	  process->state = ConcreteProcess::STATE_TERMINATED;
+          process->processTerminationCleanup();
+	  process->notifyAll ();
+	}
+      else
+	{
+	  // Unknown child.  How did this happen?
+	  fprintf (stderr, "Reaped unknown child pid = %ld\n", (long) pid);
+	}
+    }
+
+error:
+  throw new InternalError (JvNewStringUTF (strerror (errno)));
+}
+
+void
+java::lang::ConcreteProcess$ProcessManager::signalReaper ()
+{
+  int c = pthread_kill ((pthread_t) reaperPID, SIGCHLD);
+  if (c == 0)
+    return;
+  // pthread_kill() failed.
+  throw new InternalError (JvNewStringUTF (strerror (c)));
+}
+
+void
+java::lang::ConcreteProcess::nativeDestroy ()
+{
+  int c = kill ((pid_t) pid, SIGKILL);
+  if (c == 0)
+    return;
+  // kill() failed.
+  throw new InternalError (JvNewStringUTF (strerror (errno)));
+}
+
+void
+java::lang::ConcreteProcess::nativeSpawn ()
 {
   using namespace java::io;
 
-  hasExited = false;
-
   // Initialize all locals here to make cleanup simpler.
   char **args = NULL;
   char **env = NULL;
@@ -142,7 +228,6 @@ java::lang::ConcreteProcess::startProcess (jstringArray progarray,
   errp[1] = -1;
   msgp[0] = -1;
   msgp[1] = -1;
-  java::lang::Throwable *exc = NULL;
   errorStream = NULL;
   inputStream = NULL;
   outputStream = NULL;
@@ -150,8 +235,7 @@ java::lang::ConcreteProcess::startProcess (jstringArray progarray,
   try
     {
       // Transform arrays to native form.
-      args = (char **) _Jv_Malloc ((progarray->length + 1)
-				   * sizeof (char *));
+    args = (char **) _Jv_Malloc ((progarray->length + 1) * sizeof (char *));
 
       // Initialize so we can gracefully recover.
       jstring *elts = elements (progarray);
@@ -185,24 +269,32 @@ java::lang::ConcreteProcess::startProcess (jstringArray progarray,
       // status.
       if (pipe (inp) || pipe (outp) || pipe (errp) || pipe (msgp)
 	  || fcntl (msgp[1], F_SETFD, FD_CLOEXEC))
-	throw new IOException (JvNewStringLatin1 (strerror (errno)));
+      throw new IOException (JvNewStringUTF (strerror (errno)));
 
       // We create the streams before forking.  Otherwise if we had an
       // error while creating the streams we would have run the child
       // with no way to communicate with it.
-      errorStream = new FileInputStream (new FileChannelImpl(errp[0], FileChannelImpl::READ));
-      inputStream = new FileInputStream (new FileChannelImpl(inp[0], FileChannelImpl::READ));
-      outputStream = new FileOutputStream (new FileChannelImpl(outp[1], FileChannelImpl::WRITE));
+    errorStream =
+      new FileInputStream (new
+                           FileChannelImpl (errp[0], FileChannelImpl::READ));
+    inputStream =
+      new FileInputStream (new
+                           FileChannelImpl (inp[0], FileChannelImpl::READ));
+    outputStream =
+      new FileOutputStream (new FileChannelImpl (outp[1],
+                                             FileChannelImpl::WRITE));
 
       // We don't use vfork() because that would cause the local
       // environment to be set by the child.
-      if ((pid = (jlong) fork ()) == -1)
-	throw new IOException (JvNewStringLatin1 (strerror (errno)));
 
-      if (pid == 0)
+    // Use temporary for fork result to avoid dirtying an extra page.
+    pid_t pid_tmp;
+    if ((pid_tmp = fork ()) == -1)
+      throw new IOException (JvNewStringUTF (strerror (errno)));
+
+    if (pid_tmp == 0)
 	{
 	  // Child process, so remap descriptors, chdir and exec.
-
 	  if (envp)
 	    {
 	      // Preserve PATH and LD_LIBRARY_PATH unless specified
@@ -212,16 +304,16 @@ java::lang::ConcreteProcess::startProcess (jstringArray progarray,
 	      environ = env;
 	      if (path_val && getenv ("PATH") == NULL)
 		{
-		  char *path_env = (char *) _Jv_Malloc (strlen (path_val)
-							+ 5 + 1);
+		char *path_env =
+                  (char *) _Jv_Malloc (strlen (path_val) + 5 + 1);
 		  strcpy (path_env, "PATH=");
 		  strcat (path_env, path_val);
 		  putenv (path_env);
 		}
 	      if (ld_path_val && getenv ("LD_LIBRARY_PATH") == NULL)
 		{
-		  char *ld_path_env
-		    = (char *) _Jv_Malloc (strlen (ld_path_val) + 16 + 1);
+		char *ld_path_env =
+                  (char *) _Jv_Malloc (strlen (ld_path_val) + 16 + 1);
 		  strcpy (ld_path_env, "LD_LIBRARY_PATH=");
 		  strcat (ld_path_env, ld_path_val);
 		  putenv (ld_path_env);
@@ -264,6 +356,8 @@ java::lang::ConcreteProcess::startProcess (jstringArray progarray,
 
       // Parent.  Close extra file descriptors and mark ours as
       // close-on-exec.
+      pid = (jlong) pid_tmp;
+
       myclose (outp[0]);
       myclose (inp[1]);
       myclose (errp[1]);
@@ -272,9 +366,9 @@ java::lang::ConcreteProcess::startProcess (jstringArray progarray,
       char c;
       int r = read (msgp[0], &c, 1);
       if (r == -1)
-	throw new IOException (JvNewStringLatin1 (strerror (errno)));
+      throw new IOException (JvNewStringUTF (strerror (errno)));
       else if (r != 0)
-	throw new IOException (JvNewStringLatin1 (strerror (c)));
+      throw new IOException (JvNewStringUTF (strerror (c)));
     }
   catch (java::lang::Throwable *thrown)
     {
@@ -324,15 +418,13 @@ java::lang::ConcreteProcess::startProcess (jstringArray progarray,
       myclose (errp[1]);
       myclose (msgp[1]);
 
-      exc = thrown;
+    exception = thrown;
     }
 
   myclose (msgp[0]);
   cleanup (args, env, path);
 
-  if (exc != NULL)
-    throw exc;
-  else
+  if (exception == NULL)
     {
       fcntl (outp[1], F_SETFD, FD_CLOEXEC);
       fcntl (inp[0], F_SETFD, FD_CLOEXEC);