On Nov 22 09:56, Jeremy Drake via Cygwin-patches wrote: > On Fri, 22 Nov 2024, Corinna Vinschen wrote: > > > > wait_thread is happily waiting in ReadFile, while the main thread is > > > hanging in ForceCloseHandle1 (close_h, rd_proc_pipe);. > > > > ....which is one of the great annoyances of Windows. CloseHandle > > on a pipe should never hang, but... well... > > Yeah, I thought TerminateThread should never hang either, but I was proved > wrong by emulation :P > > > > The pinfo::release > > > call is after the wait thread should have been terminated, so it's > > > apparent the CancelSynchronousIo didn't work for whatever reason (possibly > > > due to canceling some other sychronous IO, > > > > How would that be possible? A thread can only run a single synchronous > > IO at the time, isn't it? > > I thought something in the rest of the logic in the thread might be doing > a synchronous IO. I dug through and found a WriteFile somewhere, but I > didn't follow the code logic to prove that that happens in the particular > cases in this thread.
Ah, ok, I misunderstood what you were trying to say and was thinking of multiple parallel synch IOs in the same thread... which I can only imagine with a certain amount of squinting... > > Rather than calling CancelSynchronousIo(), what about sending a > > signal to proc_waiter() via alert_parent(0)? > > How would that work? Wouldn't that have to be done in the child? Hmm, yeah. > > On second thought, the current behaviour after getting an > > ERROR_OPERATION_ABORTED error doesn't look right either. > > > > Rather than entering the error case, it should be exempt from > > being handled as error, just as ERROR_BROKEN_PIPE. Otherwise > > it never runs the case 0 code in the following switch statement, but > > it should. Even that change might already fix things... > > I thought this was correct, because in the case where CancelSynchronousIo > was added the thread otherwise would have been terminated. Therefore it > should exit ASAP, do not pass go, etc. Yeah, I understand the reasoning, but wasn't CancelSynchronousIo() meant to replace TerminateThread() with a *clean* thread termination? But, never mind. > > Either way, this looks like yet another synchronization problem: > > > > You can't be sure that ReadFile() actually exited after calling > > CancelSynchronousIo(). > > > > And you can't be sure that proc_waiter() was really in the ReadFile call > > when you call CancelSynchronousIo(). proc_waiter() could easily be in > > the followup code and only enter ReadFile() after your > > CancelSynchronousIo() call. > > > > All in all, it looks like calling alert_parent(0) might be the better > > approach, but ultimately, I still don't see a way around > > terminate_thread(). > > I will submit a patch reverting the CancelSynchronousIo changes, since > they were not necessary for the ARM64 fix, and were just an attempt to > avoid the necessity of using TerminateThread. Ok, thanks! Corinna
