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

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

[-- Attachment #1: Type: text/plain, Size: 2285 bytes --]

On 14/07/2023 14:04, Jon Turney wrote:
> On 13/07/2023 19:53, Corinna Vinschen wrote:
>>>>> normally after 10 seconds. (See the commentary in pthread::cancel() in
>>>>> thread.cc, where it checks if the target thread is inside the kernel,
>>>>> and silently converts the cancellation into a deferred one)
>>>>
>>>> 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.
> 
> 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.
> 
> I guess this could also check that not all of the threads ran for all 10 
> seconds, which would indicate that at least some of them were cancelled 
> asynchronously.

I wrote this, attached, which does indeed make this test more reliable.

You point is well made that this is making assumptions about how quickly 
async cancellation works that are not required by the standard

(It would be a valid, if strange implementation, if async cancellation 
always took 10 seconds to take effect, which this test assumes isn't the 
case)

Perhaps there is a better way to write a test that async cancellation 
works in the absence of cancellation points, but it eludes me...

[-- Attachment #2: 0001-Cygwin-testsuite-Make-cancel3-and-cancel5-more-robus.patch --]
[-- Type: text/plain, Size: 2819 bytes --]

From d0023fa3ea1e8f29e80d473ab13d8200bdd2dc3a Mon Sep 17 00:00:00 2001
From: Jon Turney <jon.turney@dronecode.org.uk>
Date: Sat, 15 Jul 2023 17:57:43 +0100
Subject: [PATCH] Cygwin: testsuite: Make cancel3 and cancel5 more robust

Despite our efforts, sometimes the async cancellation gets deferred.

Notice this by calling pthread_testcancel(), and then try to work out if
async cancellation was ever successful by checking if all threads ran
for the full 10 seconds, or if some were stopped early.
---
 winsup/testsuite/winsup.api/pthread/cancel3.c | 16 +++++++++++++++-
 winsup/testsuite/winsup.api/pthread/cancel5.c | 14 ++++++++++++++
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/winsup/testsuite/winsup.api/pthread/cancel3.c b/winsup/testsuite/winsup.api/pthread/cancel3.c
index 07feb7c9b..075f052cc 100644
--- a/winsup/testsuite/winsup.api/pthread/cancel3.c
+++ b/winsup/testsuite/winsup.api/pthread/cancel3.c
@@ -92,6 +92,9 @@ mythread(void * arg)
 	}
     }
 
+  /* Notice if asynchronous cancel got deferred */
+  pthread_testcancel();
+
   return result;
 }
 
@@ -101,6 +104,7 @@ main()
   int failed = 0;
   int i;
   pthread_t t[NUMTHREADS + 1];
+  int ran_to_completion = 0;
 
   assert((t[0] = pthread_self()) != NULL);
 
@@ -130,7 +134,7 @@ main()
    * Standard check that all threads started.
    */
   for (i = 1; i <= NUMTHREADS; i++)
-    { 
+    {
       if (!threadbag[i].started)
 	{
 	  failed |= !threadbag[i].started;
@@ -166,9 +170,19 @@ main()
 		  threadbag[i].count,
 		  result);
 	}
+
+      if (threadbag[i].count >= 10)
+	ran_to_completion++;
+
       failed = (failed || fail);
     }
 
+  if (ran_to_completion >= 10)
+    {
+      fprintf(stderr, "All threads ran to completion, async cancellation never happened\n");
+      failed = TRUE;
+    }
+
   assert(!failed);
 
   /*
diff --git a/winsup/testsuite/winsup.api/pthread/cancel5.c b/winsup/testsuite/winsup.api/pthread/cancel5.c
index 999b3c95c..23c02afe4 100644
--- a/winsup/testsuite/winsup.api/pthread/cancel5.c
+++ b/winsup/testsuite/winsup.api/pthread/cancel5.c
@@ -93,6 +93,9 @@ mythread(void * arg)
 	}
     }
 
+  /* Notice if asynchronous cancel got deferred */
+  pthread_testcancel();
+
   return result;
 }
 
@@ -102,6 +105,7 @@ main()
   int failed = 0;
   int i;
   pthread_t t[NUMTHREADS + 1];
+  int ran_to_completion = 0;
 
   for (i = 1; i <= NUMTHREADS; i++)
     {
@@ -165,9 +169,19 @@ main()
 		  threadbag[i].count,
 		  result);
 	}
+
+      if (threadbag[i].count >= 10)
+       ran_to_completion++;
+
       failed = (failed || fail);
     }
 
+  if (ran_to_completion >= 10)
+    {
+      fprintf(stderr, "All threads ran to completion, async cancellation never happened\n");
+      failed = TRUE;
+    }
+
   assert(!failed);
 
   /*
-- 
2.39.0