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. > 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? > 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. > 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.