On 17/07/2023 16:41, Corinna Vinschen wrote:
On Jul 17 16:21, Corinna Vinschen wrote:
On Jul 17 12:51, Jon Turney wrote:
On 17/07/2023 12:05, Corinna Vinschen wrote:
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,
Actually, it seems we actually *have* to do this. I just searched
for more info on that problem and, to my surprise, I found this in the
most obvious piece of documentation:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_exit.html
Quote:
As the meaning of the status is determined by the application (except
when the thread has been canceled, in which case it is
PTHREAD_CANCELED), [...]
On second thought...
One thing bugging me is this:
This is still a bit fuzzy, though. I'd appreciate any input.
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?
I'm not sure I follow: another thread sets cancelled = true, just before
we hit pthread::testcancel(), so we go into the WFSO, but then the other
thread continues, signals cancel_event and everything's fine.
What meaning are you assigning to "interrupted" here?
Are we worried about the thread calling pthread_cancel being cancelled
itself?
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.
Well, yeah, that is required for them to be coherent. But we have a
mutex on the thread object for that purpose, and I don't quite see why
it's released so early here.
