[PATCH v3] cygthread: suspend thread before terminating.

[Jeremy Drake]

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.