This is the mail archive of the
java-patches@gcc.gnu.org
mailing list for the Java project.
Re: [Patch] Rewrite PosixProcess
Andrew Haley wrote:
David Daney writes:
> 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?
This all seems very sensible.
A nit -- why make this change:
> @@ -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();
rather than changing addProcessToMap to DTRT? Seems like you've made
the encapsulation of ProcessManager worse for no good reason.
It was an inadvertent encapsulation breaking micro optimization.
Here is a new version of the patch that corrects this issue.
Tested as before with no failures.
OK to commit?
2007-05-11 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 and make private.
(ProcessManager.pidToProcess): Remove.
(ProcessManager.liveProcesses): New field.
(ProcessManager.reaperPID): Remove.
(ProcessManager.nativeData): New field.
(ProcessManager.removeProcessFromMap): Remove.
(ProcessManager.addProcessToMap):Remove.
(ProcessManager.addToLiveProcesses): New method.
(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.
* classpath/lib/java/lang/PosixProcess$EOFInputStream.class: Regenerate.
* classpath/lib/java/lang/PosixProcess.class: Same.
* classpath/lib/java/lang/PosixProcess$ProcessManager.class: Same.
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.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,12 @@ 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();
+ private LinkedList<PosixProcess> queue = new LinkedList<PosixProcess>();
+ private LinkedList<PosixProcess> liveProcesses =
+ new LinkedList<PosixProcess>();
private boolean ready = false;
- private long reaperPID;
+
+ static RawDataManaged nativeData;
ProcessManager()
{
@@ -53,27 +54,14 @@ 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.
+ * Add a process to the list of running processes. This must only
+ * be called with the queueLock held.
*
* @param p The PosixProcess.
*/
- void addProcessToMap(PosixProcess p)
+ void addToLiveProcesses(PosixProcess p)
{
- pidToProcess.put(new Long(p.pid), p);
+ liveProcesses.add(p);
}
/**
@@ -122,61 +110,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 +180,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 +290,7 @@ final class PosixProcess extends Process
returnedErrorStream = EOFInputStream.instance;
else
returnedErrorStream = errorStream;
-
+
return returnedErrorStream;
}
@@ -308,7 +303,7 @@ final class PosixProcess extends Process
returnedInputStream = EOFInputStream.instance;
else
returnedInputStream = inputStream;
-
+
return returnedInputStream;
}
@@ -329,7 +324,7 @@ final class PosixProcess extends Process
/**
* Start this process running. This should only be called by the
- * ProcessManager.
+ * ProcessManager with the queueLock held.
*
* @param pm The ProcessManager that made the call.
*/
@@ -342,7 +337,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.addToLiveProcesses(this);
state = STATE_RUNNING;
// Notify anybody waiting on state change.
this.notifyAll();
@@ -426,7 +421,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/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)
{