From 6827aee0a2aced980d90055b0c2fb181c7a66546 Mon Sep 17 00:00:00 2001
From: Sam Clegg <sbc@chromium.org>
Date: Tue, 3 Sep 2024 10:59:55 -0700
Subject: [PATCH] Feedback from #22467. NFC (#22479)

---
 system/lib/pthread/pthread_kill.c | 10 +++++-----
 test/pthread/test_pthread_kill.c  | 20 +++++++++++---------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/system/lib/pthread/pthread_kill.c b/system/lib/pthread/pthread_kill.c
index 767fe01ce4c1..86e486cea9ae 100644
--- a/system/lib/pthread/pthread_kill.c
+++ b/system/lib/pthread/pthread_kill.c
@@ -17,17 +17,17 @@ void do_raise(void* arg) {
   int sig = (intptr_t)arg;
   if (sig == SIGCANCEL) {
     // For `SIGCANCEL` there is no need to actually call raise to run the
-    // handler function.  The calling then (the one calling `pthread_cancel`)
+    // handler function.  The calling thread (the one calling `pthread_cancel`)
     // will already have marked us as being cancelled.  All we need to do is
     // ensure that `pthread_testcancel` is eventually called and that will cause
     // this thread to exit.  We can't call `pthread_testcancel` here (since we
     // are being called from the proxy queue process and we don't want to leave
     // that in a bad state by unwinding).  Instead, we rely on
     // `pthread_testcancel` at the end of `_emscripten_check_mailbox`.  Before
-    // we return though we do want to make sure we clear the keepalive state so
-    // that the thread will exit even if it has a reason to stay alive.
-    // TODO(sbc): Is this the correct behaviour, should `pthread_cancel`
-    // instead wait for threads to be done with outstanding work/event loops?
+    // we return, we do want to make sure we clear the keepalive state so that
+    // the thread will exit even if it has a reason to stay alive.  TODO(sbc):
+    // Is this the correct behaviour, should `pthread_cancel` instead wait for
+    // threads to be done with outstanding work/event loops?
     _emscripten_runtime_keepalive_clear();
     return;
   }
diff --git a/test/pthread/test_pthread_kill.c b/test/pthread/test_pthread_kill.c
index 37bbc6cf8c54..531de0d65a12 100644
--- a/test/pthread/test_pthread_kill.c
+++ b/test/pthread/test_pthread_kill.c
@@ -12,15 +12,16 @@
 #include <errno.h>
 #include <signal.h>
 
-_Atomic int sharedVar = 0;
-_Atomic int gotTermSignal = 0;
+pthread_cond_t started_cond = PTHREAD_COND_INITIALIZER;
+pthread_mutex_t started_lock = PTHREAD_MUTEX_INITIALIZER;
+_Atomic int got_term_signal = 0;
 
 pthread_t thr;
 
 void signal_handler(int sig, siginfo_t * info, void * arg) {
   printf("signal: %d onthread=%d\n", sig, pthread_self() == thr);
   if (sig == SIGTERM) {
-    gotTermSignal = 1;
+    got_term_signal = 1;
   }
 }
 
@@ -38,10 +39,12 @@ void sleepms(long msecs) {
 }
 
 void *thread_start(void *arg) {
+  pthread_mutex_lock(&started_lock);
+  pthread_cond_signal(&started_cond);
+  pthread_mutex_unlock(&started_lock);
   // As long as this thread is running, keep the shared variable latched to nonzero value.
-  while (!gotTermSignal) {
+  while (!got_term_signal) {
     sleepms(1);
-    ++sharedVar;
   }
   printf("got term signal, shutting down thread\n");
   pthread_exit(0);
@@ -50,14 +53,13 @@ void *thread_start(void *arg) {
 int main() {
   setup_handler();
 
-  sharedVar = 0;
   int s = pthread_create(&thr, NULL, thread_start, 0);
   assert(s == 0);
 
   // Wait until thread kicks in and sets the shared variable.
-  while (sharedVar == 0) {
-    sleepms(10);
-  }
+  pthread_mutex_lock(&started_lock);
+  pthread_cond_wait(&started_cond, &started_lock);
+  pthread_mutex_unlock(&started_lock);
   printf("thread has started, sending SIGTERM\n");
 
   s = pthread_kill(thr, SIGTERM);