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

[Jon Turney <jon.turney@dronecode.org.uk>]

On 17/07/2023 12:05, Corinna Vinschen wrote:
> On Jul 14 20:57, Corinna Vinschen wrote:
>> On Jul 14 14:04, Jon Turney wrote:
>>> On 13/07/2023 19:53, Corinna Vinschen wrote:
>>>>>> Nevertheless, I think this is ok to do.  The description of pthread_cancel
>>>>>> contains this:
>>>>>>
>>>>>>     Asynchronous cancelability means that the thread can be canceled at
>>>>>>     any time (usually immediately, but the system does not guarantee this).
>>>>>>
>>>>>> And
>>>>>>
>>>>>>     The above steps happen asynchronously with respect to the
>>>>>>     pthread_cancel() call; the return status of pthread_cancel() merely
>>>>>>     informs the caller whether the cancellation request was successfully
>>>>>>     queued.
>>>>>>
>>>>>> So any assumption *when* the cancallation takes place is may be wrong.
>>>
>>> Yeah.
>>>
>>> I think the flakiness is when we happen to try to async cancel while in the
>>> Windows kernel, which implicitly converts to a deferred cancellation, but
>>> there are no cancellation points in the the thread, so it arrives at
>>> pthread_exit() and returns a exit code other than PTHREAD_CANCELED.
>>
>> In pthread_join(), right?
>>
>>> I did consider making the test non-flaky by adding a final call to
>>> pthread_testcancel(), to notice any failed async cancellation which has been
>>> converted to a deferred one.
>>>
>>> But then that is just the same as the deferred cancellation tests, and
>>> confirms the cancellation happens, but not that it's async, which is part of
>>> the point of the test.
>>
>> 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...

It doesn't seem a bad addition, 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.