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.

diff --git a/src/ChangeLog b/src/ChangeLog
index 2723d8f92ece4797a4f78250572b6765e11d0ab3..c7502104ddffbcfddaed90d619530acd57300810 100644 (file)
--- 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 aa3431a13574cc420a47162b164712140e62c6c5..47b950668b08727b48ec1f04030964206ee7c102 100644 (file)
--- 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 1693b2ad5018379162580be6f3fc35689fa25a83..5c43a57db29568fde5001a794a796eafaf1867cb 100644 (file)
--- 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);
 }