* cygthread.cc (cygthread::release): Reset ev here if it exists.

(cygthread::terminate_thread): Eliminat racy code which reset ev and
thread_sync.  Remove a few nonsensical inuse checks.  Exit at the bottom.
(cygthread::detach): Rewrite to again try to ensure that we don't say we're
signalled when we are not signalled.
* fhandler.cc (fhandler_base::raw_read): Revert to signalling read success
quickly.
* pipe.cc (fhandler_pipe::close): Use base method to close handle.
* sigproc.h (WAIT_SIG_PRIORITY): Just trundle along at normal priority to allow
the pipe thread to do its thing if possible.
* pinfo.h (pinfo::zap_cwd): Declare new function.
(pinfo::zap_cwd): Move 'cd out of the way code' here.
(pinfo::exit): Use it here.
* spawn.cc (spawn_guts): And here.

winsup/cygwin/ChangeLog

@@ -1,3 +1,22 @@
+2005-02-11  Christopher Faylor  <cgf@timesys.com>
+
+	* cygthread.cc (cygthread::release): Reset ev here if it exists.
+	(cygthread::terminate_thread): Eliminat racy code which reset ev and
+	thread_sync.  Remove a few nonsensical inuse checks.  Exit at the
+	bottom.
+	(cygthread::detach): Rewrite to again try to ensure that we don't say
+	we're signalled when we are not signalled.
+	* fhandler.cc (fhandler_base::raw_read): Revert to signalling read
+	success quickly.
+	* pipe.cc (fhandler_pipe::close): Use base method to close handle.
+	* sigproc.h (WAIT_SIG_PRIORITY): Just trundle along at normal priority
+	to allow the pipe thread to do its thing if possible.
+
+	* pinfo.h (pinfo::zap_cwd): Declare new function.
+	(pinfo::zap_cwd): Move 'cd out of the way code' here.
+	(pinfo::exit): Use it here.
+	* spawn.cc (spawn_guts): And here.
+
 2005-02-11  Corinna Vinschen  <corinna@vinschen.de>
 
 	* times.cc (utimes): Open files with GENERIC_WRITE on file systems

winsup/cygwin/cygthread.cc

@@ -234,6 +234,8 @@ cygthread::release (bool nuke_h)
 #endif
   __name = NULL;
   func = NULL;
+  if (ev)
+    ResetEvent (ev);
   if (!InterlockedExchange (&inuse, 0))
 #ifdef DEBUGGING
     api_fatal ("released a thread that was not inuse");
@@ -247,37 +249,22 @@ bool
 cygthread::terminate_thread ()
 {
   bool terminated = true;
-  /* FIXME: The if (!inuse) stuff below should be handled better.  The
-     problem is that terminate_thread could be called while a thread
-     is terminating and either the thread could be handling its own
-     release or, if this is being called during exit, some other
-     thread may be attempting to free up this resource.  In the former
-     case, setting some kind of "I deal with my own exit" type of
-     flag may be the way to handle this. */
-  if (!is_freerange)
-    {
-      ResetEvent (*this);
-      ResetEvent (thread_sync);
-    }
-
   debug_printf ("thread '%s', id %p, inuse %d, stack_ptr %p", name (), id, inuse, stack_ptr);
   while (inuse && !stack_ptr)
     low_priority_sleep (0);
 
   if (!inuse)
-    return false;
+    goto force_notterminated;
     
   (void) TerminateThread (h, 0);
   (void) WaitForSingleObject (h, INFINITE);
-  if (ev)
-    terminated = WaitForSingleObject (ev, 0) != WAIT_OBJECT_0;
-  if (!inuse || exiting)
-    return false;
-
   CloseHandle (h);
 
-  if (!inuse)
+  if (!inuse || exiting)
-    return false;
+    goto force_notterminated;
+
+  if (ev)
+    terminated = WaitForSingleObject (ev, 0) != WAIT_OBJECT_0;
 
   MEMORY_BASIC_INFORMATION m;
   memset (&m, 0, sizeof (m));
@@ -289,8 +276,6 @@ cygthread::terminate_thread ()
     debug_printf ("VirtualFree of allocation base %p<%p> failed, %E",
 		   stack_ptr, m.AllocationBase);
 
-  if (!inuse)
-    /* nothing */;
   if (is_freerange)
     free (this);
   else
@@ -300,6 +285,12 @@ cygthread::terminate_thread ()
 #endif
       release (true);
     }
+  
+  goto out;
+
+force_notterminated:
+  terminated = false;
+out:
   return terminated;
 }
 
@@ -311,7 +302,7 @@ bool
 cygthread::detach (HANDLE sigwait)
 {
   bool signalled = false;
-  bool terminated = false;
+  bool thread_was_reset = false;
   if (!inuse)
     system_printf ("called detach but inuse %d, thread %p?", inuse, id);
   else
@@ -322,35 +313,61 @@ cygthread::detach (HANDLE sigwait)
 	res = WaitForSingleObject (*this, INFINITE);
       else
 	{
+	  /* Lower our priority and give priority to the read thread */
+	  HANDLE hth = GetCurrentThread ();
+	  LONG prio = GetThreadPriority (hth);
+	  (void) ::SetThreadPriority (hth, THREAD_PRIORITY_IDLE);
+
 	  HANDLE w4[2];
-	  w4[0] = *this;
+	  unsigned n = 2;
+	  DWORD howlong = INFINITE;
+	  w4[0] = sigwait;
 	  w4[1] = signal_arrived;
-	  res = WaitForSingleObject (sigwait, INFINITE);
+	  /* For a description of the below loop see the end of this file */
-	  if (res != WAIT_OBJECT_0)
+	  for (int i = 0; i < 2; i++)
-	    system_printf ("WFSO sigwait %p failed, res %u, %E", sigwait, res);
+	    switch (res = WaitForMultipleObjects (n, w4, FALSE, howlong))
-	  res = WaitForMultipleObjects (2, w4, FALSE, INFINITE);
+	      {
-	  if (res == WAIT_OBJECT_0)
+	      case WAIT_OBJECT_0:
-	    signalled = false;
+		if (n == 1)
-	  else if (res != WAIT_OBJECT_0 + 1)
+		  howlong = 50;
+		break;
+	      case WAIT_OBJECT_0 + 1:
+		n = 1;
+		if (i--)
+		  howlong = 50;
+		break;
+	      case WAIT_TIMEOUT:
+		break;
+	      default:
+		if (!exiting)
 		  api_fatal ("WFMO failed waiting for cygthread '%s'", __name);
-	  else if ((res = WaitForSingleObject (*this, 0)) == WAIT_OBJECT_0)
+		break;
-	    signalled = false;
+	      }
+	  /* WAIT_OBJECT_0 means that the thread successfully read something,
+	     so wait for the cygthread to "terminate". */
+	  if (res == WAIT_OBJECT_0)
+	    (void) WaitForSingleObject (*this, INFINITE);
 	  else
 	    {
-	      terminated = true;
+	      /* Thread didn't terminate on its own, so maybe we have to
+		 do it. */
 	      signalled = terminate_thread ();
-	    }
+	      /* Possibly the thread completed *just* before it was
+		 terminated.  Detect this. If this happened then the
+		 read was not terminated on a signal. */
 	      if (WaitForSingleObject (sigwait, 0) == WAIT_OBJECT_0)
 		signalled = false;
-	  else if (signalled)
+	      if (signalled)
-	    set_sig_errno (EINTR);    /* caller should be dealing with return
+		set_sig_errno (EINTR);
-					 values. */
+	      thread_was_reset = true;
+	    }
+	  (void) ::SetThreadPriority (hth, prio);
 	}
 
       thread_printf ("%s returns %d, id %p", sigwait ? "WFMO" : "WFSO",
 		     res, id);
 
-      if (terminated)
+      if (thread_was_reset)
 	/* already handled */;
       else if (is_freerange)
 	{
@@ -372,3 +389,71 @@ cygthread::terminate ()
 {
   exiting = 1;
 }
+
+/* The below is an explanation of synchronization loop in cygthread::detach.
+   The intent is that the loop will always try hard to wait for both
+   synchronization events from the reader thread but will exit with
+   res == WAIT_TIMEOUT if a signal occurred and the reader thread is
+   still blocked.
+
+    case 0 - no signal
+
+    i == 0 (howlong == INFINITE)
+	W0 activated
+	howlong not set because n != 1
+	just loop
+
+    i == 1 (howlong == INFINITE)
+	W0 activated
+	howlong not set because n != 1
+	just loop (to exit loop) - no signal
+
+    i == 2 (howlong == INFINITE)
+	exit loop
+
+    case 1 - signal before thread initialized
+
+    i == 0 (howlong == INFINITE)
+	WO + 1 activated
+	n set to 1
+	howlong untouched because i-- == 0
+	loop
+
+    i == 0 (howlong == INFINITE)
+	W0 must be activated
+	howlong set to 50 because n == 1
+
+    i == 1 (howlong == 50)
+	W0 activated
+	loop (to exit loop) - no signal
+
+	WAIT_TIMEOUT activated
+	signal potentially detected
+	loop (to exit loop)
+
+    i == 2 (howlong == 50)
+	exit loop
+
+    case 2 - signal after thread initialized
+
+    i == 0 (howlong == INFINITE)
+	W0 activated
+	howlong not set because n != 1
+	loop
+
+    i == 1 (howlong == INFINITE)
+	W0 + 1 activated
+	n set to 1
+	howlong set to 50 because i-- != 0
+	loop
+
+    i == 1 (howlong == 50)
+	W0 activated
+	loop (to exit loop) - no signal
+
+	WAIT_TIMEOUT activated
+	loop (to exit loop) - signal
+
+    i == 2 (howlong == 50)
+	exit loop
+*/

winsup/cygwin/fhandler.cc

@@ -234,6 +234,11 @@ fhandler_base::raw_read (void *ptr, size_t& ulen)
       signal_read_state (1);
     }
   BOOL res = ReadFile (get_handle (), ptr, len, (DWORD *) &ulen, 0);
+  if (read_state)
+    {
+      signal_read_state (1);
+      (void) SetThreadPriority (h, prio);
+    }
   if (!res)
     {
       /* Some errors are not really errors.  Detect such cases here.  */
@@ -270,11 +275,6 @@ fhandler_base::raw_read (void *ptr, size_t& ulen)
 	  break;
 	}
     }
-  if (read_state)
-    {
-      signal_read_state (1);
-      (void) SetThreadPriority (h, prio);
-    }
 #undef bytes_read
 }
 

winsup/cygwin/pinfo.cc

@@ -123,6 +123,16 @@ pinfo::maybe_set_exit_code_from_windows ()
 		  self->pid, oexitcode, x, self->exitcode);
 }
 
+void
+pinfo::zap_cwd ()
+{
+  extern char windows_system_directory[];
+  /* Move to an innocuous location to avoid a race with other processes
+     that may want to manipulate the current directory before this
+     process has completely exited.  */
+  (void) SetCurrentDirectory (windows_system_directory);
+}
+
 void
 pinfo::exit (DWORD n)
 {
@@ -144,11 +154,7 @@ pinfo::exit (DWORD n)
 
   if (n != EXITCODE_NOSET)
     {
-      extern char windows_system_directory[];
+      zap_cwd ();
-      /* Move to an innocuous location to avoid a race with other processes
-	 that may want to manipulate the current directory before this
-	 process has completely exited.  */
-      (void) SetCurrentDirectory (windows_system_directory);
       self->alert_parent (0);		/* Shave a little time by telling our
 					   parent that we have now exited.  */
     }

winsup/cygwin/pinfo.h

@@ -197,6 +197,7 @@ public:
 #endif
   HANDLE shared_handle () {return h;}
   void set_acl ();
+  void zap_cwd ();
   friend class _pinfo;
 };
 

winsup/cygwin/pipe.cc

@@ -210,12 +210,7 @@ fhandler_pipe::close ()
   if (read_state && !cygheap->fdtab.in_vfork_cleanup ())
 #endif
     ForceCloseHandle (read_state);
-  if (get_handle ())
+  return fhandler_base::close ();
-    {
-      CloseHandle (get_handle ());
-      set_io_handle (NULL);
-    }
-  return 0;
 }
 
 bool

winsup/cygwin/sigproc.h

@@ -88,7 +88,7 @@ extern char myself_nowait_dummy[];
 
 extern struct sigaction *global_sigs;
 
-#define WAIT_SIG_PRIORITY THREAD_PRIORITY_TIME_CRITICAL
+#define WAIT_SIG_PRIORITY THREAD_PRIORITY_NORMAL
 
 #define myself_nowait ((_pinfo *)myself_nowait_dummy)
 #endif /*_SIGPROC_H*/

winsup/cygwin/spawn.cc

@@ -804,7 +804,7 @@ spawn_guts (const char * prog_arg, const char *const *argv,
 
 	 If wr_proc_pipe exists, then it should be duplicated to the child.
 	 If the child has exited already, that's ok.  The parent will pick up
-	 on this fact when we exit.  dup_proc_pipe also closes our end of the pipe.
+	 on this fact when we exit.  dup_proc_pipe will close our end of the pipe.
 	 Note that wr_proc_pipe may also be == INVALID_HANDLE_VALUE.  That will make
 	 dup_proc_pipe essentially a no-op.  */
       if (myself->wr_proc_pipe)
@@ -812,11 +812,7 @@ spawn_guts (const char * prog_arg, const char *const *argv,
 	  myself->sync_proc_pipe ();	/* Make sure that we own wr_proc_pipe
 					   just in case we've been previously
 					   execed. */
-	  SetCurrentDirectory ("c:\\");	/* Move to an innocuous location to
+	  myself.zap_cwd ();
-					   avoid races with other processes
-					   that may want to manipulate the
-					   current directory before this process
-					   has completely exited.  */
 	  (void) myself->dup_proc_pipe (pi.hProcess);
 	}
     }