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

[David Daney <ddaney at avtrex dot com>]

Bryce McKinlay wrote:
> 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.
> 

Agreed.

> 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.
> 

I think there is a fundamental problem with mixing java.lang.Process and
other native code that is either catching SIGCHLD and/or wait()ing.  No
amount of hand waving can make it go away.  If two threads unmask
SIGCHLD then they will get each other's signals.  Likewise wait()ing
will be screwed up.

I think we should just say that it doesn't work and prohibit it.  The
current implementation has the same problems, so we would not be making
it any worse.

> 
>> // 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.


The only problem I see is more configure/automake hackery.  I will
probably end up breaking the Win32 port in the process.


> 
> 
>>!         /**
>>!          * 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?
> 

Yes, they need better names.


> 
>>!     	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.
> 

The problem I see with this is that given the current naming scheme of
ConcreteProcess, there would then be a declaration of
ConcreteProcess$ProcessManager even for non-Posix platforms.  However if
we rename to PosixProcess, Win32Process, etc. then this might not be an
issue.


>>+ // 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?

I should be, but it makes me feel better to see it explicitly done in
the same file that is using the signal.

I suppose that it can go.

> 
> 
>>!         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.
> 

In that case things are badly broken.  Again, I think there may be
insurmountable problems in allowing people to do this.

David Daney.