Improve handling of subprocess shutdown on MS-Windows.

 src/w32proc.c (reader_thread): Do not index fd_info[] with negative
 values.
 (reader_thread): Exit when cp->status becomes STATUS_READ_ERROR
 after WaitForSingleObject returns normally.  This expedites reader
 thread shutdown when delete_child triggers it.
 (reap_subprocess): More accurate commentary for why we call
 delete_child only when cp->fd is negative.
 src/w32.c (sys_close): Do not call delete_child on a subprocess
 whose handle is not yet closed.  Instead, set its file descriptor
 to a negative value, so that reap_subprocess will call
 delete_child on that subprocess when its SIGCHLD arrives.  This
 avoids closing handles used for communications between sys_select
 and reader_thread, which doesn't give sys_select a chance to
 notice that the process exited and invoke the SIGCHLD handler for
 it.

3 files changed, 42 insertions, 7 deletions

diff --git a/src/ChangeLog b/src/ChangeLog
index 2723d8f92ec..c7502104ddf 100644
--- a/src/ChangeLog
+++ b/src/ChangeLog
@@ -1,3 +1,22 @@
+2012-12-23  Eli Zaretskii  <eliz@gnu.org>
+
+        * w32proc.c (reader_thread): Do not index fd_info[] with negative
+        values.
+        (reader_thread): Exit when cp->status becomes STATUS_READ_ERROR
+        after WaitForSingleObject returns normally.  This expedites reader
+        thread shutdown when delete_child triggers it.
+        (reap_subprocess): More accurate commentary for why we call
+        delete_child only when cp->fd is negative.
+
+        * w32.c (sys_close): Do not call delete_child on a subprocess
+        whose handle is not yet closed.  Instead, set its file descriptor
+        to a negative value, so that reap_subprocess will call
+        delete_child on that subprocess when its SIGCHLD arrives.  This
+        avoids closing handles used for communications between sys_select
+        and reader_thread, which doesn't give sys_select a chance to
+        notice that the process exited and invoke the SIGCHLD handler for
+        it.
+
 2012-12-23  Jan Djärv  <jan.h.d@swipnet.se>
 
         * nsfns.m (Fns_do_applescript): Run event loop until script has
diff --git a/src/w32.c b/src/w32.c
index aa3431a1357..47b950668b0 100644
--- a/src/w32.c
+++ b/src/w32.c
@@ -6401,7 +6401,21 @@ sys_close (int fd)
 
                   winsock_inuse--; /* count open sockets */
                 }
-              delete_child (cp);
+              /* If the process handle is NULL, it's either a socket
+                 or serial connection, or a subprocess that was
+                 already reaped by reap_subprocess, but whose
+                 resources were not yet freed, because its output was
+                 not fully read yet by the time it was reaped.  (This
+                 usually happens with async subprocesses whose output
+                 is being read by Emacs.)  Otherwise, this process was
+                 not reaped yet, so we set its FD to a negative value
+                 to make sure sys_select will eventually get to
+                 calling the SIGCHLD handler for it, which will then
+                 invoke waitpid and reap_subprocess.  */
+              if (cp->procinfo.hProcess == NULL)
+                delete_child (cp);
+              else
+                cp->fd = -1;
             }
         }
     }
diff --git a/src/w32proc.c b/src/w32proc.c
index 1693b2ad501..5c43a57db29 100644
--- a/src/w32proc.c
+++ b/src/w32proc.c
@@ -965,7 +965,7 @@ reader_thread (void *arg)
     {
       int rc;
 
-      if (fd_info[cp->fd].flags & FILE_LISTEN)
+      if (cp->fd >= 0 && fd_info[cp->fd].flags & FILE_LISTEN)
         rc = _sys_wait_accept (cp->fd);
       else
         rc = _sys_read_ahead (cp->fd);
@@ -993,6 +993,8 @@ reader_thread (void *arg)
                      "%lu for fd %ld\n", GetLastError (), cp->fd));
           break;
         }
+      if (cp->status == STATUS_READ_ERROR)
+        break;
     }
   return 0;
 }
@@ -1163,11 +1165,11 @@ reap_subprocess (child_process *cp)
       cp->procinfo.hThread = NULL;
     }
 
-  /* For asynchronous children, the child_proc resources will be freed
-     when the last pipe read descriptor is closed; for synchronous
-     children, we must explicitly free the resources now because
-     register_child has not been called. */
-  if (cp->fd == -1)
+  /* If cp->fd was not closed yet, we might be still reading the
+     process output, so don't free its resources just yet.  The call
+     to delete_child on behalf of this subprocess will be made by
+     sys_read when the subprocess output is fully read.  */
+  if (cp->fd < 0)
     delete_child (cp);
 }