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.
