[PATCH v3] cygthread: suspend thread before terminating.

[Corinna Vinschen]

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