Fixing thread exit and thread cancellation

src/library_pthread.js

@@ -173,6 +173,30 @@ var LibraryPThread = {
       if (ENVIRONMENT_IS_PTHREAD && _pthread_self()) ___pthread_tsd_run_dtors();
     },
 
+    runExitHandlersAndDeinitThread: function(tb, exitCode) {
+#if PTHREADS_PROFILING
+      var profilerBlock = Atomics.load(HEAPU32, (tb + {{{ C_STRUCTS.pthread.profilerBlock }}} ) >> 2);
+      Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.profilerBlock }}} ) >> 2, 0);
+      _free(profilerBlock);
+#endif
+
+      // Disable all cancellation so that executing the cleanup handlers won't trigger another JS
+      // canceled exception to be thrown.
+      Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.canceldisable }}} ) >> 2, 1/*PTHREAD_CANCEL_DISABLE*/);
+      Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.cancelasync }}} ) >> 2, 0/*PTHREAD_CANCEL_DEFERRED*/);
+      PThread.runExitHandlers();
+
+      Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadExitCode }}} ) >> 2, exitCode);
+      // When we publish this, the main thread is free to deallocate the thread object and we are done.
+      // Therefore set _pthread_self = 0; above to 'release' the object in this worker thread.
+      Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadStatus }}} ) >> 2, 1); // Mark the thread as no longer running.
+
+      _emscripten_futex_wake(tb + {{{ C_STRUCTS.pthread.threadStatus }}}, {{{ cDefine('INT_MAX') }}}); // wake all threads
+
+      // Not hosting a pthread anymore in this worker, reset the info structures to null.
+      __emscripten_thread_init(0, 0, 0); // Unregister the thread block also inside the asm.js scope.
+    },
+
     // Called when we are performing a pthread_exit(), either explicitly called
     // by programmer, or implicitly when leaving the thread main function.
     threadExit: function(exitCode) {
@@ -181,24 +205,8 @@ var LibraryPThread = {
 #if ASSERTIONS
         err('Pthread 0x' + tb.toString(16) + ' exited.');
 #endif
-#if PTHREADS_PROFILING
-        var profilerBlock = Atomics.load(HEAPU32, (tb + {{{ C_STRUCTS.pthread.profilerBlock }}} ) >> 2);
-        Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.profilerBlock }}} ) >> 2, 0);
-        _free(profilerBlock);
-#endif
-        Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadExitCode }}} ) >> 2, exitCode);
-        // When we publish this, the main thread is free to deallocate the thread object and we are done.
-        // Therefore set _pthread_self = 0; above to 'release' the object in this worker thread.
-        Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadStatus }}} ) >> 2, 1);
-
-        // Disable all cancellation so that executing the cleanup handlers won't trigger another JS
-        // canceled exception to be thrown.
-        Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.canceldisable }}} ) >> 2, 1/*PTHREAD_CANCEL_DISABLE*/);
-        Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.cancelasync }}} ) >> 2, 0/*PTHREAD_CANCEL_DEFERRED*/);
-        PThread.runExitHandlers();
-
-        _emscripten_futex_wake(tb + {{{ C_STRUCTS.pthread.threadStatus }}}, {{{ cDefine('INT_MAX') }}});
-        __emscripten_thread_init(0, 0, 0); // Unregister the thread block also inside the asm.js scope.
+        PThread.runExitHandlersAndDeinitThread(tb, exitCode);
+
         if (ENVIRONMENT_IS_PTHREAD) {
           // Note: in theory we would like to return any offscreen canvases back to the main thread,
           // but if we ever fetched a rendering context for them that would not be valid, so we don't try.
@@ -208,13 +216,7 @@ var LibraryPThread = {
     },
 
     threadCancel: function() {
-      PThread.runExitHandlers();
-      var tb = _pthread_self();
-      Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadExitCode }}} ) >> 2, -1/*PTHREAD_CANCELED*/);
-      Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.threadStatus }}} ) >> 2, 1); // Mark the thread as no longer running.
-      _emscripten_futex_wake(tb + {{{ C_STRUCTS.pthread.threadStatus }}}, {{{ cDefine('INT_MAX') }}}); // wake all threads
-      // Not hosting a pthread anymore in this worker, reset the info structures to null.
-      __emscripten_thread_init(0, 0, 0); // Unregister the thread block also inside the asm.js scope.
+      PThread.runExitHandlersAndDeinitThread(_pthread_self(), -1/*PTHREAD_CANCELED*/);
       postMessage({ 'cmd': 'cancelDone' });
     },
 
@@ -494,9 +496,23 @@ var LibraryPThread = {
   $cleanupThread: function(pthread_ptr) {
     if (ENVIRONMENT_IS_PTHREAD) throw 'Internal Error! cleanupThread() can only ever be called from main application thread!';
     if (!pthread_ptr) throw 'Internal Error! Null pthread_ptr in cleanupThread!';
-    {{{ makeSetValue('pthread_ptr', C_STRUCTS.pthread.self, 0, 'i32') }}};
     var pthread = PThread.pthreads[pthread_ptr];
+    // If pthread has been removed from this map this also means that pthread_ptr points
+    // to already freed data. Such situation may occur in following circumstances:
+    // 1. Joining cancelled thread - in such situation it may happen that pthread data will
+    //    already be removed by handling 'cancelDone' message.
+    // 2. Joining thread from non-main browser thread (this also includes thread running main()
+    //    when compiled with `PROXY_TO_PTHREAD`) - in such situation it may happen that following
+    //    code flow occur (MB - Main Browser Thread, S1, S2 - Worker Threads):
+    //    S2: thread ends, 'exit' message is sent to MB
+    //    S1: calls pthread_join(S2), this causes:
+    //        a. S2 is marked as detached,
+    //        b. 'cleanupThread' message is sent to MB.
+    //    MB: handles 'exit' message, as thread is detached, so returnWorkerToPool()
+    //        is called and all thread related structs are freed/released.
+    //    MB: handles 'cleanupThread' message which calls this function.
     if (pthread) {
+      {{{ makeSetValue('pthread_ptr', C_STRUCTS.pthread.self, 0, 'i32') }}};
       var worker = pthread.worker;
       PThread.returnWorkerToPool(worker);
     }

system/lib/libc/musl/src/thread/__timedwait.c

@@ -42,7 +42,12 @@ int __timedwait_cp(volatile int *addr, int val,
 #ifdef __EMSCRIPTEN__
 	double msecsToSleep = top ? (top->tv_sec * 1000 + top->tv_nsec / 1000000.0) : INFINITY;
 	int is_main_thread = emscripten_is_main_browser_thread();
-	if (is_main_thread || pthread_self()->cancelasync == PTHREAD_CANCEL_ASYNCHRONOUS) {
+	// cp suffix in the function name means "cancellation point", so this wait can be cancelled
+	// by the users unless current threads cancelability is set to PTHREAD_CANCEL_DISABLE
+	// which may be either done by the user of __timedwait() function.
+	if (is_main_thread ||
+	    pthread_self()->canceldisable != PTHREAD_CANCEL_DISABLE ||
+	    pthread_self()->cancelasync == PTHREAD_CANCEL_ASYNCHRONOUS) {
 		double sleepUntilTime = emscripten_get_now() + msecsToSleep;
 		do {
 			if (_pthread_isduecanceled(pthread_self())) {

system/lib/libc/musl/src/thread/pthread_barrier_wait.c

@@ -87,14 +87,29 @@ int pthread_barrier_wait(pthread_barrier_t *b)
 		while (spins-- && !inst->finished)
 			a_spin();
 		a_inc(&inst->finished);
-		while (inst->finished == 1) {
 #ifdef __EMSCRIPTEN__
-			emscripten_futex_wait(&inst->finished, 1, INFINITY);
+		int is_main_thread = emscripten_is_main_browser_thread();
+		while (inst->finished == 1) {
+			if (is_main_thread) {
+				int e;
+				do {
+					// Main thread waits in _very_ small slices so that it stays responsive to assist proxied
+					// pthread calls.
+					e = emscripten_futex_wait(&inst->finished, 1, 1);
+					// Assist other threads by executing proxied operations that are effectively singlethreaded.
+					emscripten_main_thread_process_queued_calls();
+				} while(e == -ETIMEDOUT);
+			} else {
+				// Can wait in one go.
+				emscripten_futex_wait(&inst->finished, 1, INFINITY);
+			}
+		}
 #else
+		while (inst->finished == 1) {
 			__syscall(SYS_futex,&inst->finished,FUTEX_WAIT|128,1,0) != -ENOSYS
 			|| __syscall(SYS_futex,&inst->finished,FUTEX_WAIT,1,0);
-#endif
 		}
+#endif
 		return PTHREAD_BARRIER_SERIAL_THREAD;
 	}
 

tests/pthread/test_pthread_cancel_cond_wait.cpp

@@ -0,0 +1,87 @@
+// Copyright 2021 The Emscripten Authors.  All rights reserved.
+// Emscripten is available under two separate licenses, the MIT license and the
+// University of Illinois/NCSA Open Source License.  Both these licenses can be
+// found in the LICENSE file.
+
+#include <pthread.h>
+#include <sys/types.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <assert.h>
+#include <unistd.h>
+#include <errno.h>
+#include <emscripten.h>
+#include <emscripten/threading.h>
+
+pthread_barrier_t barrier;
+pthread_mutex_t mutex = PTHREAD_MUTEX_INITIALIZER;
+pthread_cond_t condvar = PTHREAD_COND_INITIALIZER;
+
+int th_cancelled = 0;
+
+volatile int res = 43;
+
+static void cleanup_handler(void *arg) {
+  emscripten_log(EM_LOG_CONSOLE, "Called clean-up handler with arg %p", arg);
+  int a = reinterpret_cast<int>(arg);
+  res -= a;
+
+  pthread_mutex_unlock(&mutex);
+  pthread_barrier_wait(&barrier);
+}
+
+static void *thread_start(void *arg) {
+  pthread_cleanup_push(cleanup_handler, (void*)42);
+  emscripten_log(EM_LOG_CONSOLE, "Thread started!");
+  pthread_mutex_lock(&mutex);
+  pthread_barrier_wait(&barrier);
+
+  int ret = 0;
+  do {
+    emscripten_log(EM_LOG_CONSOLE, "Waiting on conditional variable");
+    ret = pthread_cond_wait(&condvar, &mutex);
+  } while (ret == 0 && th_cancelled == 0);
+
+  if (ret != 0) {
+    emscripten_log(EM_LOG_CONSOLE, "Cond wait failed ret: %d", ret);
+  }
+
+  res = 1000; // Shouldn't ever reach here.
+  pthread_cleanup_pop(0);
+
+  pthread_mutex_unlock(&mutex);
+  pthread_barrier_wait(&barrier);
+  return NULL;
+}
+
+int main() {
+  if (!emscripten_has_threading_support()) {
+    printf("Skipped: Threading is not supported.\n");
+    return 1;
+  }
+
+  pthread_barrier_init(&barrier, nullptr, 2);
+
+  pthread_t thr;
+  int s = pthread_create(&thr, NULL, thread_start, (void*)0);
+  assert(s == 0);
+  emscripten_log(EM_LOG_CONSOLE, "Thread created");
+
+  pthread_barrier_wait(&barrier);
+
+  // Lock mutex to ensure that thread is waiting
+  pthread_mutex_lock(&mutex);
+
+  emscripten_log(EM_LOG_CONSOLE, "Canceling thread..");
+  s = pthread_cancel(thr);
+  assert(s == 0);
+  th_cancelled = 1;
+  pthread_mutex_unlock(&mutex);
+
+  emscripten_log(EM_LOG_CONSOLE, "Main thread waitnig for side-thread");
+  pthread_barrier_wait(&barrier);
+  pthread_barrier_destroy(&barrier);
+
+  emscripten_log(EM_LOG_CONSOLE, "Test finished result: %d", res);
+  return res;
+}

tests/test_browser.py

@@ -3771,6 +3771,11 @@ def test_std_thread_detach(self):
   def test_pthread_cancel(self):
     self.btest(path_from_root('tests', 'pthread', 'test_pthread_cancel.cpp'), expected='1', args=['-O3', '-s', 'USE_PTHREADS', '-s', 'PTHREAD_POOL_SIZE=8'])
 
+  # Test that pthread_cancel() cancels pthread_cond_wait() operation
+  @requires_threads
+  def test_pthread_cancel_cond_wait(self):
+    self.btest_exit(path_from_root('tests', 'pthread', 'test_pthread_cancel_cond_wait.cpp'), expected='1', args=['-O3', '-s', 'USE_PTHREADS=1', '-s', 'PTHREAD_POOL_SIZE=8'])
+
   # Test pthread_kill() operation
   @no_chrome('pthread_kill hangs chrome renderer, and keep subsequent tests from passing')
   @requires_threads

tests/test_core.py

@@ -8148,7 +8148,6 @@ def test_pthread_c11_threads(self):
       self.set_setting('INITIAL_MEMORY', '64mb')
     self.do_run_in_out_file_test('tests', 'pthread', 'test_pthread_c11_threads.c')
 
-  @no_asan('flakey errors that must be fixed, https://github.com/emscripten-core/emscripten/issues/12985')
   @node_pthreads
   def test_pthread_cxx_threads(self):
     self.set_setting('PROXY_TO_PTHREAD')

tests/test_posixtest.py

@@ -147,10 +147,7 @@ def get_pthread_tests():
   **flaky,
   'test_pthread_create_11_1': 'never returns',
   'test_pthread_barrier_wait_2_1': 'never returns',
-  'test_pthread_cond_timedwait_2_6': 'never returns',
-  'test_pthread_cond_timedwait_4_3': 'never returns',
   'test_pthread_attr_setscope_5_1': 'internally skipped (PTS_UNTESTED)',
-  'test_pthread_cond_wait_2_3': 'never returns',
   'test_pthread_create_5_1': 'never returns',
   'test_pthread_exit_1_2': 'never returns',
   'test_pthread_exit_2_2': 'never returns',