Re: [RFT/RFC] Fix PosixProcess / PR libgcj 11801...

[Bryce McKinlay <mckinlay at redhat dot com>]

Hi David, this looks really good. Thanks for working on it. Here are my 
comments.

David Daney wrote:

> *** prims.cc	24 Jul 2004 01:17:28 -0000	1.95
> --- prims.cc	27 Jul 2004 18:32:46 -0000
> *************** _Jv_RunMain (jclass klass, const char *n
> *** 1068,1073 ****
> --- 1068,1086 ----
>        runtime->exit (1);
>      }
>  
> + #ifdef _POSIX_VERSION
> +   // PosixProcess uses SIGCHLD internally.  It is important that
> +   // SIGCHLD is only handled there so we mask it here so the mask can
> +   // be inherited by all other threads.  The idea is for SIGCHLD to be
> +   // masked in all threads so it can be guaranteed to be delivered to
> +   // the sigwait() in the process reaper thread.
> +   sigset_t sig_mask;
> + 
> +   sigemptyset (&sig_mask);
> +   sigaddset (&sig_mask, SIGCHLD);
> +   sigprocmask (SIG_BLOCK, &sig_mask, NULL);
> + #endif
> + 
>  
This won't work when the runtime is started with JvCreateJavaVM() or for 
native threads which are attached to the runtime by 
JvAttachCurrentThread, etc. Instead, this should be done from 
posix-threads.cc - I think it would have to be called from both 
_Jv_ThreadStart() and _Jv_ThreadRegister() to cover both the 
thread-creating and attach-existing-thread cases. By putting it in 
posix-threads, we avoid the need for the #ifdef and platform-specific 
code in prims.cc, too.

Also, I think pthread_sigmask should be used in preference to 
sigprocmask() - apparently they do exactly the same thing, but 
pthread_sigmask is better style.

A concern here is what happens when the runtime is started by a native 
process that has its own threads that it doesn't register with libgcj.  
I don't know of an easy way to ensure that those other threads block 
SIGCHLD. One possibility would be to intercept its pthread_create() 
calls (boehm-gc actually already does this), but thats a bit ugly and 
non-portable - it would be good to come up with a portable solution if 
one exists.

>  // Hence the class name apparently does not match the file name.
>  final class ConcreteProcess extends Process
>  {
>  

Perhaps we should just call this class PosixProcess and build it 
conditionally. Then we can just have Runtime.execInternal() create the 
right one depending on which platform we're on. Any reasons not to do 
that? It would have the advantage of avoiding the missing class errors 
from other compilers/tools.

> !         /**
> !          * 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 processForPid(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 addToProcessMap(ConcreteProcess p)
> !         {
> !             pidToProcess.put(new Long(p.pid), p);
> !         }
>  
Just a nit, but for clarity and better abstraction of implementation 
details, wouldn't something like "removeProcess()" and "addProcess()" be 
better names for these?

> !     	if(null == progarray[0]) {
>  

For these comparisons, "if (progarray[0] == null)" is the preferred 
style. There are several places that this should be changed.

> + 
> + // FIXME: Should ConcreteProcess$ProcessManager be added to javaprims.h?
> + extern "Java"
> + {
> +   namespace java
> +   {
> +     namespace lang
> +     {
> +         class ConcreteProcess$ProcessManager;
> +     }
> +   }
> + }
> +

Yes - run scripts/classes.pl to regenerate the namespace declarations in 
javaprims.h, then you can get rid of this.

> + static void
> + sigchld_handler(int sig)
> + {
> +     // Don't cause compiler warnings.
> +     (void)sig;
> +     // Ignore.
>  

A better way to avoid the warning is just to not name the parameter. eg:

 sigchld_handler(int)
 {
   ...
 }

> + // Get ready to enter the main reaper thread loop.
>  void
> ! java::lang::ConcreteProcess$ProcessManager::init ()
>  {
> !     using namespace java::lang;
> !     // Remenber our PID so other threads can kill us.
> !     reaperPID = (jlong)pthread_self();
> ! 
> !     // Block SIGCHLD as we are about to enable handling of it.
> !     sigset_t mask;
> !     sigemptyset(&mask);
> !     sigaddset(&mask, SIGCHLD);
> !     int c = pthread_sigmask(SIG_BLOCK, &mask, NULL);
>  

This shouldn't be needed here - won't SIGCHLD already be blocked?

> !         if(process) {
> !             JvSynchronize sync(process);
> !             process->status = WIFEXITED (status) ? WEXITSTATUS (status) : -1;
> !             process->state = ConcreteProcess::STATE_TERMINATED;
> !             process->notifyAll();
> !         }
> !         else {
> !             // Unknown child.  How did this happen?
> !             fprintf(stderr, "Reaped unknown child pid = %ld\n", (long)pid);
> !         }
> !     }
>  

Perhaps this could happen if another non-GCJ thread was spawning 
children. There is probably no harm in keeping this warning for now, though.

Regards

Bryce