Re: [RFA] Patch: Fix PosixProcess / PR libgcj 11801

[Bryce McKinlay <mckinlay at redhat dot com>]

Hi David,

Sorry for the slow review. See below for my comments.

David Daney wrote:

> *************** _Jv_ThreadRegister (_Jv_Thread_t *data)
> *** 358,363 ****
> --- 359,371 ----
>  	_Jv_self_cache[current_index].high_sp_bits = BAD_HIGH_SP_VALUE;
>        }
>  # endif
> +   // Block SIGCHLD which is used in natPosixProcess.cc.
> +   sigset_t mask;
> +   sigemptyset (&mask);
> +   sigaddset (&mask, SIGCHLD);
> +   int c = pthread_sigmask (SIG_BLOCK, &mask, NULL);
> +   if (c != 0)
> +     throw new java::lang::InternalError (JvNewStringLatin1 (strerror (c)));
>  }
>  
>  void
> *************** _Jv_ThreadStart (java::lang::Thread *thr
> *** 403,408 ****
> --- 411,425 ----
>      return;
>    data->flags |= FLAG_START;
>  
> +   // Block SIGCHLD which is used in natPosixProcess.cc.
> +   // The current mask is inherited by the child thread.
> +   sigset_t mask;
> +   sigemptyset (&mask);
> +   sigaddset (&mask, SIGCHLD);
> +   int c = pthread_sigmask (SIG_BLOCK, &mask, NULL);
> +   if (c != 0)
> +     throw new java::lang::InternalError (JvNewStringLatin1 (strerror (c)));
> + 
>    param.sched_priority = thread->getPriority();
>  

These should be moved into a common function, called from both 
_Jv_ThreadRegister and _Jv_ThreadStart.

>  
> !     /**
> !      * 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);
> ! 	    }
> !         }
> !     }
>  

Is the outer try block here necessary? I think it would be better just 
to let the default handler for the thread handle it.

> !   /**
> !    * 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
> !    * generated an IOException due to SIGPIPE.
> !    *
> !    * 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()
> !   {
> !     try
> !       {
> !         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.
> !           }
> !       }
> !     catch (Exception ex)
> !       {
> !         // Not an IOException.  Something bad happened.
> !         InternalError ie = new InternalError(ex.toString());
> !         ie.initCause(ex);
> !         throw ie;
> !       }
>  

Again, I think the outer catch block is unneccessary here. No need to 
re-throw because we already caught the only checked exceptions?

> ! 
> ! error:
> !   throw new InternalError (JvNewStringLatin1 (strerror (c)));
> ! }
>  

Minor nit, but these should all use JvNewStringUtf8(), because those 
error messages are internationalized.

Also, EOFInputStream could be a singleton?

Apart from these minor issues, this patch is looking good. I'm still a 
bit concerned about the SIGCHLD hander limitations, however - I think we 
should mention this in the CNI documentation in gcj.texi?

Regards

Bryce