Re: [PATCH 08/11] Cygwin: testsuite: Busy-wait in cancel3 and cancel5

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

On Jul 17 12:51, Jon Turney wrote:
> On 17/07/2023 12:05, Corinna Vinschen wrote:
> > On Jul 14 20:57, Corinna Vinschen wrote:
> > > What if Cygwin checks for a deferred cancellation in pthread::exit,
> > > too?  It needs to do this by its own, not calling pthread::testcancel,
> > > otherwise we're in an infinite loop.  Since cancel is basically like
> > > exit, just with a PTHREAD_CANCELED return value, the only additional
> > > action would be to set retval to PTHREAD_CANCELED explicitely.
> > 
> > Kind of like this:
> > 
> > diff --git a/winsup/cygwin/thread.cc b/winsup/cygwin/thread.cc
> > index f614e01c42f6..fceb9bda1806 100644
> > --- a/winsup/cygwin/thread.cc
> > +++ b/winsup/cygwin/thread.cc
> > @@ -546,6 +546,13 @@ pthread::exit (void *value_ptr)
> >     class pthread *thread = this;
> >     _cygtls *tls = cygtls;	/* Save cygtls before deleting this. */
> > +  /* Deferred cancellation still pending? */
> > +  if (canceled)
> > +    {
> > +      WaitForSingleObject (cancel_event, INFINITE);
> > +      value_ptr = PTHREAD_CANCELED;
> > +    }
> > +
> >     // run cleanup handlers
> >     pop_all_cleanup_handlers ();
> > What do you think?
> 
> I mean, by your own interpretation of the standard, this isn't required,
> because we're allowed to take arbitrarily long to deliver the async
> cancellation, and in this case, we took so long that the thread exited
> before it happened, too bad...

True enough!

> It doesn't seem a bad addition,

On second thought...

One thing bugging me is this:

Looking into pthread::cancel we have this order of things:

    // cancel deferred
    mutex.unlock ();
    canceled = true;
    SetEvent (cancel_event);
    return 0; 

The canceled var is set before the SetEvent call.
What if the thread is terminated after canceled is set to true but
before SetEvent is called?

pthread::testcancel claims:

  We check for the canceled flag first. [...]
  Only if the thread is marked as canceled, we wait for cancel_event
  being really set, on the off-chance that pthread_cancel gets
  interrupted before calling SetEvent.

Neat idea to speed up the code, but doesn't that mean we have a
potential deadlock, especially given that pthread::testcancel calls WFSO
with an INFINITE timeout?

And if so, how do we fix this?  Theoretically, the most simple
solution might be to call SetEvent before setting the canceled
variable, but in fact we would have to make setting canceld
and cancel_event an atomic operation.

Another idea is never to wait for an INFINITE time.  Logically, if
canceled is set and cancel_event isn't, the thread just hasn't been
canceled yet.

Any better idea?

> but this turns pthread_exit() into a
> deferred cancellation point as well, so it should be added to the list of
> "an implementation may also mark other functions not specified in the
> standard as cancellation points" in our documentation^W the huge comment in
> threads.cc.

Yes, indeed.


Thanks,
Corinna