Re: [PATCH] [MinGW]: ConcreteProcess Refactoring and Pipe Bugfix (Updated to fix PR/12231)

[Mohan Embar <gnustuff at thisiscool dot com>]

Hi People,

>Here is an updated version of the patch that fixes the flashing
>console window issue on MinGW:
>
>http://gcc.gnu.org/bugzilla/show_bug.cgi?id=12231
>
>The only change from the previous version of this patch:
>
>http://gcc.gnu.org/ml/java-patches/2003-q4/msg00253.html
>
>...is passing CREATE_NO_WINDOW to CreateProcess().

This is slightly redone in terms of the _Jv_close_on_exec patch:

http://gcc.gnu.org/ml/java-patches/2003-q4/msg00306.html

Again, I'm going to let this age on the list for awhile.

-- Mohan
http://www.thisiscool.com/
http://www.animalsong.org/

ChangeLog
2003-10-30  Mohan Embar  <gnustuff@thisiscool.com>

	PR libgcj/12231
	* java/lang/Win32Process.java (hasExited) Changed from
	public to private.
	(startProcess): Likewise.
	(cleanup): Likewise.
	* java/lang/natWin32Process.cc (cleanup) Don't close
	input, output and error streams.
	(ChildProcessPipe): New helper class.
	(startProcess): Refactored to use ChildProcessPipe.
	Use CREATE_NO_WINDOW when launching child process.

Index: java/lang/Win32Process.java
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/Win32Process.java,v
retrieving revision 1.7
diff -u -2 -r1.7 Win32Process.java
--- java/lang/Win32Process.java	27 Jul 2003 04:13:03 -0000	1.7
+++ java/lang/Win32Process.java	28 Oct 2003 22:08:30 -0000
@@ -29,6 +29,4 @@
   public native void destroy ();
 
-  public native boolean hasExited ();
-
   public int exitValue ()
   {
@@ -56,11 +54,4 @@
   public native int waitFor () throws InterruptedException;
 
-  public native void startProcess (String[] progarray,
-				   String[] envp,
-				   File dir)
-    throws IOException;
-
-  public native void cleanup ();
-
   public ConcreteProcess (String[] progarray,
                           String[] envp,
@@ -90,3 +81,10 @@
   // Exit code of the child if it has exited.
   private int exitCode;
+
+  private native boolean hasExited ();
+  private native void startProcess (String[] progarray,
+           String[] envp,
+           File dir)
+    throws IOException;
+  private native void cleanup ();
 }
Index: java/lang/natWin32Process.cc
===================================================================
RCS file: /cvs/gcc/gcc/libjava/java/lang/natWin32Process.cc,v
retrieving revision 1.6
diff -u -2 -r1.6 natWin32Process.cc
--- java/lang/natWin32Process.cc	19 Sep 2003 08:28:43 -0000	1.6
+++ java/lang/natWin32Process.cc	28 Oct 2003 22:08:31 -0000
@@ -30,21 +30,27 @@
 java::lang::ConcreteProcess::cleanup (void)
 {
-  if (inputStream != NULL)
-    {
-      inputStream->close ();
-      inputStream = NULL;
-    }
-
-  if (outputStream != NULL)
-    {
-      outputStream->close ();
-      outputStream = NULL;
-    }
-
-  if (errorStream != NULL)
-    {
-      errorStream->close ();
-      errorStream = NULL;
-    }
+  // FIXME:
+  // We used to close the input, output and
+  // error streams here, but we can't do that
+  // because the caller also has the right
+  // to close these and FileInputStream and FileOutputStream
+  // scream if you attempt to close() them twice. Presently,
+  // we use _Jv_platform_close_on_exec, which is similar
+  // to the POSIX approach.
+  //
+  // What I wanted to do is have private nested
+  // classes in ConcreteProcess which extend FileInputStream
+  // and FileOutputStream, respectively, but override
+  // close() to permit multiple calls to close(). This
+  // led to class header and platform configury issues
+  // that I didn't feel like dealing with. However,
+  // this approach could conceivably be a good multiplatform
+  // one since delaying the pipe close until process
+  // termination could be wasteful if many child processes
+  // are spawned within the parent process' lifetime.
+  inputStream = NULL;
+  outputStream = NULL;
+  errorStream = NULL;
+  
   if (procHandle)
     {
@@ -130,4 +136,74 @@
 }
 
+
+// Helper class for creating and managing the pipes
+// used for I/O redirection for child processes.
+class ChildProcessPipe
+{
+public:
+  // Indicates from the child process' point of view
+  // whether the pipe is for reading or writing.
+  enum EType {INPUT, OUTPUT};
+
+  ChildProcessPipe(EType eType);
+  ~ChildProcessPipe();
+  
+  // Returns a pipe handle suitable for use by the parent process
+  HANDLE getParentHandle();
+  
+  // Returns a pipe handle suitable for use by the child process.
+  HANDLE getChildHandle();
+  
+private:
+  EType m_eType;
+  HANDLE m_hRead, m_hWrite;
+};
+
+ChildProcessPipe::ChildProcessPipe(EType eType):
+  m_eType(eType)
+{
+  SECURITY_ATTRIBUTES sAttrs;
+
+  // Explicitly allow the handles to the pipes to be inherited.
+  sAttrs.nLength = sizeof (SECURITY_ATTRIBUTES);
+  sAttrs.bInheritHandle = 1;
+  sAttrs.lpSecurityDescriptor = NULL;
+
+  if (CreatePipe (&m_hRead, &m_hWrite, &sAttrs, 0) == 0)
+    {
+      DWORD dwErrorCode = GetLastError ();
+      throw new java::io::IOException (
+        _Jv_WinStrError ("Error creating pipe", dwErrorCode));
+    }
+
+  // If this is the read end of the child, we need
+  // to make the parent write end non-inheritable. Similarly,
+  // if this is the write end of the child, we need to make
+  // the parent read end non-inheritable. If we didn't
+  // do this, the child would inherit these ends and we wouldn't
+  // be able to close them from our end. For full details,
+  // do a Google search on "Q190351".
+  HANDLE& rhStd = m_eType==INPUT ? m_hWrite : m_hRead;
+  _Jv_platform_close_on_exec (rhStd);
+}
+
+ChildProcessPipe::~ChildProcessPipe()
+{
+  // Close the parent end of the pipe. This
+  // destructor is called after the child process
+  // has been spawned.
+  CloseHandle(getChildHandle());
+}
+
+HANDLE ChildProcessPipe::getParentHandle()
+{
+  return m_eType==INPUT ? m_hWrite : m_hRead;
+}
+
+HANDLE ChildProcessPipe::getChildHandle()
+{
+  return m_eType==INPUT ? m_hRead : m_hWrite;
+}
+
 void
 java::lang::ConcreteProcess::startProcess (jstringArray progarray,
@@ -198,44 +274,14 @@
       // We create anonymous pipes to communicate with the child
       // on each of standard streams.
-
-      HANDLE cldStdInRd, cldStdInWr;
-      HANDLE cldStdOutRd, cldStdOutWr;
-      HANDLE cldStdErrRd, cldStdErrWr;
-
-      SECURITY_ATTRIBUTES sAttrs;
-
-      // Explicitly allow the handles to the pipes to be inherited.
-      sAttrs.nLength = sizeof (SECURITY_ATTRIBUTES);
-      sAttrs.bInheritHandle = 1;
-      sAttrs.lpSecurityDescriptor = NULL;
-
-
-      if (CreatePipe (&cldStdInRd, &cldStdInWr, &sAttrs, 0) == 0)
-        {
-          DWORD dwErrorCode = GetLastError ();
-          throw new IOException (_Jv_WinStrError ("Error creating stdin pipe",
-            dwErrorCode));
-        }
-
-      if (CreatePipe (&cldStdOutRd, &cldStdOutWr, &sAttrs, 0) == 0)
-        {
-          DWORD dwErrorCode = GetLastError ();
-          throw new IOException (_Jv_WinStrError ("Error creating stdout pipe",
-            dwErrorCode));
-        }
-
-      if (CreatePipe (&cldStdErrRd, &cldStdErrWr, &sAttrs, 0) == 0)
-        {
-          DWORD dwErrorCode = GetLastError ();
-          throw new IOException (_Jv_WinStrError ("Error creating stderr pipe",
-            dwErrorCode));
-        }
-
-      outputStream = new FileOutputStream
-                         (new FileDescriptor ((jint) cldStdInWr));
-      inputStream = new FileInputStream
-                        (new FileDescriptor ((jint) cldStdOutRd));
-      errorStream = new FileInputStream
-                        (new FileDescriptor ((jint) cldStdErrRd));
+      ChildProcessPipe aChildStdIn(ChildProcessPipe::INPUT);
+      ChildProcessPipe aChildStdOut(ChildProcessPipe::OUTPUT);
+      ChildProcessPipe aChildStdErr(ChildProcessPipe::OUTPUT);
+
+      outputStream = new FileOutputStream (new FileDescriptor (
+                           (jint) aChildStdIn.getParentHandle ()));
+      inputStream = new FileInputStream (new FileDescriptor (
+                           (jint) aChildStdOut.getParentHandle ()));
+      errorStream = new FileInputStream (new FileDescriptor (
+                           (jint) aChildStdErr.getParentHandle ()));
 
       // Now create the child process.
@@ -251,8 +297,12 @@
       si.dwFlags |= STARTF_USESTDHANDLES;
 
-      si.hStdInput = cldStdInRd;
-      si.hStdOutput = cldStdOutWr;
-      si.hStdError = cldStdErrWr;
-
+      si.hStdInput = aChildStdIn.getChildHandle();
+      si.hStdOutput = aChildStdOut.getChildHandle();
+      si.hStdError = aChildStdErr.getChildHandle();
+
+      // Spawn the process. CREATE_NO_WINDOW only applies when
+      // starting a console application; it suppresses the
+      // creation of a console window. This flag is ignored on
+      // Win9X.
       if (CreateProcess (NULL,
                          cmdLine,
@@ -260,5 +310,5 @@
                          NULL,
                          1,
-                         0,
+                         CREATE_NO_WINDOW,
                          env,
                          wdir,
@@ -272,9 +322,4 @@
 
       procHandle = (jint ) pi.hProcess;
-
-      // Close the wrong ends (for the parent) of the pipes.
-      CloseHandle (cldStdInRd);
-      CloseHandle (cldStdOutWr);
-      CloseHandle (cldStdErrWr);
 
       _Jv_Free (cmdLine);