From: Eli Zaretskii <eliz@gnu.org>
Date: Sun, 23 Dec 2012 17:06:58 +0000 (+0200)
Subject: Improve handling of subprocess shutdown on MS-Windows.
X-Git-Tag: emacs-24.3.90~173^2~7^2~511
X-Git-Url: http://git.eshelyaron.com/gitweb/?a=commitdiff_plain;h=299614f3bcac13be9a17d038f247856e384d9dbd;p=emacs.git

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 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);
 }