diff options
| -rw-r--r-- | winsup/cygwin/ChangeLog | 19 | ||||
| -rw-r--r-- | winsup/cygwin/cygthread.cc | 169 | ||||
| -rw-r--r-- | winsup/cygwin/fhandler.cc | 10 | ||||
| -rw-r--r-- | winsup/cygwin/pinfo.cc | 16 | ||||
| -rw-r--r-- | winsup/cygwin/pinfo.h | 3 | ||||
| -rw-r--r-- | winsup/cygwin/pipe.cc | 7 | ||||
| -rw-r--r-- | winsup/cygwin/sigproc.h | 2 | ||||
| -rw-r--r-- | winsup/cygwin/spawn.cc | 8 |
8 files changed, 168 insertions, 66 deletions
diff --git a/winsup/cygwin/ChangeLog b/winsup/cygwin/ChangeLog index e70b9402e..c0d120815 100644 --- a/winsup/cygwin/ChangeLog +++ b/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 diff --git a/winsup/cygwin/cygthread.cc b/winsup/cygwin/cygthread.cc index 4c14f1954..a5e52be00 100644 --- a/winsup/cygwin/cygthread.cc +++ b/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) - return false; + if (!inuse || exiting) + 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); - if (res != WAIT_OBJECT_0) - system_printf ("WFSO sigwait %p failed, res %u, %E", sigwait, res); - res = WaitForMultipleObjects (2, w4, FALSE, INFINITE); + /* For a description of the below loop see the end of this file */ + for (int i = 0; i < 2; i++) + switch (res = WaitForMultipleObjects (n, w4, FALSE, howlong)) + { + case WAIT_OBJECT_0: + if (n == 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); + break; + } + /* WAIT_OBJECT_0 means that the thread successfully read something, + so wait for the cygthread to "terminate". */ if (res == WAIT_OBJECT_0) - signalled = false; - else if (res != WAIT_OBJECT_0 + 1) - api_fatal ("WFMO failed waiting for cygthread '%s'", __name); - else if ((res = WaitForSingleObject (*this, 0)) == WAIT_OBJECT_0) - signalled = false; + (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; + if (signalled) + set_sig_errno (EINTR); + thread_was_reset = true; } - if (WaitForSingleObject (sigwait, 0) == WAIT_OBJECT_0) - signalled = false; - else if (signalled) - set_sig_errno (EINTR); /* caller should be dealing with return - values. */ + (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 +*/ diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc index 1a18b7550..ab588b500 100644 --- a/winsup/cygwin/fhandler.cc +++ b/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 } diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc index 367ecd187..eedb69ad5 100644 --- a/winsup/cygwin/pinfo.cc +++ b/winsup/cygwin/pinfo.cc @@ -124,6 +124,16 @@ pinfo::maybe_set_exit_code_from_windows () } 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) { exit_state = ES_FINAL; @@ -144,11 +154,7 @@ pinfo::exit (DWORD n) if (n != EXITCODE_NOSET) { - 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); + zap_cwd (); self->alert_parent (0); /* Shave a little time by telling our parent that we have now exited. */ } diff --git a/winsup/cygwin/pinfo.h b/winsup/cygwin/pinfo.h index 7a502c100..45bc406cd 100644 --- a/winsup/cygwin/pinfo.h +++ b/winsup/cygwin/pinfo.h @@ -196,7 +196,8 @@ public: } #endif HANDLE shared_handle () {return h;} - void set_acl(); + void set_acl (); + void zap_cwd (); friend class _pinfo; }; diff --git a/winsup/cygwin/pipe.cc b/winsup/cygwin/pipe.cc index eab600aff..48499f4bc 100644 --- a/winsup/cygwin/pipe.cc +++ b/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 ()) - { - CloseHandle (get_handle ()); - set_io_handle (NULL); - } - return 0; + return fhandler_base::close (); } bool diff --git a/winsup/cygwin/sigproc.h b/winsup/cygwin/sigproc.h index 020d43884..bc6f3cf2b 100644 --- a/winsup/cygwin/sigproc.h +++ b/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*/ diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc index 1f30175da..b83a7dfe2 100644 --- a/winsup/cygwin/spawn.cc +++ b/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 - avoid races with other processes - that may want to manipulate the - current directory before this process - has completely exited. */ + myself.zap_cwd (); (void) myself->dup_proc_pipe (pi.hProcess); } } |