Simplify thread clean / exit / terminate logic. NFC.

Mostly inspired by musl.

Author: kleisauke

src/library_pthread.js

@@ -29,7 +29,6 @@
 var LibraryPThread = {
   $PThread__postset: 'PThread.init();',
   $PThread__deps: ['_emscripten_thread_init',
-                   '$killThread',
                    '$cancelThread', '$cleanupThread', '$zeroMemory',
                    '$ptrToString', '$spawnThread',
                    '_emscripten_thread_free_data',
@@ -157,7 +156,7 @@ var LibraryPThread = {
       for (var t in PThread.pthreads) {
         var pthread = PThread.pthreads[t];
         if (pthread && pthread.worker) {
-          PThread.returnWorkerToPool(pthread.worker);
+          cleanupThread(t, true);
         }
       }
 
@@ -178,17 +177,21 @@ var LibraryPThread = {
       }
       PThread.unusedWorkers = [];
     },
-    returnWorkerToPool: function(worker) {
+    returnWorkerToPool: function(worker, terminate) {
       // We don't want to run main thread queued calls here, since we are doing
       // some operations that leave the worker queue in an invalid state until
       // we are completely done (it would be bad if free() ends up calling a
       // queued pthread_create which looks at the global data structures we are
       // modifying).
       PThread.runWithoutMainThreadQueuedCalls(function() {
         delete PThread.pthreads[worker.pthread.threadInfoStruct];
-        // Note: worker is intentionally not terminated so the pool can
-        // dynamically grow.
-        PThread.unusedWorkers.push(worker);
+        if (terminate) {
+          worker.terminate();
+        } else {
+          // No termination needed? Return it to the worker pool as an
+          // unused worker so the pool can dynamically grow.
+          PThread.unusedWorkers.push(worker);
+        }
         PThread.runningWorkers.splice(PThread.runningWorkers.indexOf(worker), 1);
         // Not a running Worker anymore
         __emscripten_thread_free_data(worker.pthread.threadInfoStruct);
@@ -272,9 +275,7 @@ var LibraryPThread = {
         } else if (cmd === 'spawnThread') {
           spawnThread(d);
         } else if (cmd === 'cleanupThread') {
-          cleanupThread(d['thread']);
-        } else if (cmd === 'killThread') {
-          killThread(d['thread']);
+          cleanupThread(d['thread'], d['terminate']);
         } else if (cmd === 'cancelThread') {
           cancelThread(d['thread']);
         } else if (cmd === 'loaded') {
@@ -451,59 +452,23 @@ var LibraryPThread = {
     }
   },
 
-  $killThread__deps: ['_emscripten_thread_free_data'],
-  $killThread: function(pthread_ptr) {
-#if PTHREADS_DEBUG
-    err('killThread ' + ptrToString(pthread_ptr));
-#endif
-#if ASSERTIONS
-    assert(!ENVIRONMENT_IS_PTHREAD, 'Internal Error! killThread() can only ever be called from main application thread!');
-    assert(pthread_ptr, 'Internal Error! Null pthread_ptr in killThread!');
-#endif
-    {{{ makeSetValue('pthread_ptr', C_STRUCTS.pthread.self, 0, 'i32') }}};
-    var pthread = PThread.pthreads[pthread_ptr];
-    delete PThread.pthreads[pthread_ptr];
-    pthread.worker.terminate();
-    __emscripten_thread_free_data(pthread_ptr);
-    // The worker was completely nuked (not just the pthread execution it was hosting), so remove it from running workers
-    // but don't put it back to the pool.
-    PThread.runningWorkers.splice(PThread.runningWorkers.indexOf(pthread.worker), 1); // Not a running Worker anymore.
-    pthread.worker.pthread = undefined;
-  },
-
   __emscripten_thread_cleanup: function(thread) {
     // Called when a thread needs to be cleaned up so it can be reused.
     // A thread is considered reusable when it either returns from its
     // entry point, calls pthread_exit, or acts upon a cancellation.
     // Detached threads are responsible for calling this themselves,
     // otherwise pthread_join is responsible for calling this.
-    if (!ENVIRONMENT_IS_PTHREAD) cleanupThread(thread);
-    else postMessage({ 'cmd': 'cleanupThread', 'thread': thread });
+    if (!ENVIRONMENT_IS_PTHREAD) cleanupThread(thread, false);
+    else postMessage({ 'cmd': 'cleanupThread', 'thread': thread, 'terminate': false });
   },
 
-  $cleanupThread: function(pthread_ptr) {
+  $cleanupThread: function(pthread_ptr, terminate) {
 #if ASSERTIONS
     assert(!ENVIRONMENT_IS_PTHREAD, 'Internal Error! cleanupThread() can only ever be called from main application thread!');
     assert(pthread_ptr, 'Internal Error! Null pthread_ptr in cleanupThread!');
 #endif
     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 circumstance:
-    // 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);
-    }
+    if (pthread) PThread.returnWorkerToPool(pthread.worker, terminate);
   },
 
 #if MAIN_MODULE
@@ -555,7 +520,7 @@ var LibraryPThread = {
     assert(pthread_ptr, 'Internal Error! Null pthread_ptr in cancelThread!');
 #endif
     var pthread = PThread.pthreads[pthread_ptr];
-    pthread.worker.postMessage({ 'cmd': 'cancel' });
+    if (pthread) pthread.worker.postMessage({ 'cmd': 'cancel' });
   },
 
   $spawnThread: function(threadParams) {
@@ -842,17 +807,17 @@ var LibraryPThread = {
       err('pthread_kill attempted on a null thread pointer!');
       return {{{ cDefine('ESRCH') }}};
     }
-    var self = {{{ makeGetValue('thread', C_STRUCTS.pthread.self, 'i32') }}};
-    if (self !== thread) {
+    var tid = {{{ makeGetValue('thread', C_STRUCTS.pthread.tid, 'i32') }}};
+    if (!tid) {
       err('pthread_kill attempted on thread ' + ptrToString(thread) + ', which does not point to a valid thread, or does not exist anymore!');
       return {{{ cDefine('ESRCH') }}};
     }
     if (signal === {{{ cDefine('SIGCANCEL') }}}) { // Used by pthread_cancel in musl
       if (!ENVIRONMENT_IS_PTHREAD) cancelThread(thread);
       else postMessage({ 'cmd': 'cancelThread', 'thread': thread });
     } else if (signal != 0) {
-      if (!ENVIRONMENT_IS_PTHREAD) killThread(thread);
-      else postMessage({ 'cmd': 'killThread', 'thread': thread });
+      if (!ENVIRONMENT_IS_PTHREAD) cleanupThread(thread, true);
+      else postMessage({ 'cmd': 'cleanupThread', 'thread': thread, 'terminate': true });
     }
     return 0;
   },

system/lib/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp

@@ -528,7 +528,7 @@ tid_t GetTid() {
 #elif SANITIZER_SOLARIS
   return thr_self();
 #elif SANITIZER_EMSCRIPTEN
-  return (tid_t) pthread_self();
+  return gettid();
 #else
   return internal_syscall(SYSCALL(gettid));
 #endif

system/lib/libc/musl/src/internal/pthread_impl.h

@@ -77,8 +77,8 @@ struct pthread {
 #ifdef __EMSCRIPTEN__
 	// If --threadprofiler is enabled, this pointer is allocated to contain
 	// internal information about the thread state for profiling purposes.
-	thread_profiler_block * _Atomic profilerBlock;
-	int stack_owned;
+	thread_profiler_block *profilerBlock;
+	volatile int stack_owned;
 #endif
 };
 

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

@@ -6,7 +6,7 @@ static int __pthread_detach(pthread_t t)
 #ifdef __EMSCRIPTEN__
 	// XXX EMSCRIPTEN: Add check for invalid (already joined) thread.  Again
 	// for the benefit of the conformance tests.
-	if (t->self != t) return ESRCH;
+	if (!t->tid) return ESRCH;
 #endif
 	/* If the cas fails, detach state is either already-detached
 	 * or exiting/exited, and pthread_join will trap or cleanup. */

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

@@ -16,7 +16,7 @@ static int __pthread_timedjoin_np(pthread_t t, void **res, const struct timespec
 #ifdef __EMSCRIPTEN__
 	// Attempt to join a thread which does not point to a valid thread, or
 	// does not exist anymore.
-	if (t->self != t) return ESRCH;
+	if (!t->tid) return ESRCH;
 	// Thread is attempting to join to itself.  Already detached threads are
 	// handled below by returning EINVAL instead.
 	// TODO: The detached check here is just to satisfy the `other.test_{proxy,main}_pthread_join_detach` tests.

system/lib/pthread/library_pthread.c

@@ -653,8 +653,7 @@ __attribute__((constructor(48)))
 void __emscripten_init_main_thread(void) {
   __emscripten_init_main_thread_js(&__main_pthread);
 
-  // The pthread struct has a field that points to itself - this is used as
-  // a magic ID to detect whether the pthread_t structure is 'alive'.
+  // The pthread struct has a field that points to itself.
   __main_pthread.self = &__main_pthread;
   __main_pthread.stack = (void*)emscripten_stack_get_base();
   __main_pthread.stack_size = emscripten_stack_get_base() - emscripten_stack_get_end();

system/lib/pthread/pthread_create.c

@@ -104,9 +104,10 @@ int __pthread_create(pthread_t* restrict res,
   // zero-initialize thread structure.
   memset(new, 0, sizeof(struct pthread));
 
-  // The pthread struct has a field that points to itself - this is used as a
-  // magic ID to detect whether the pthread_t structure is 'alive'.
+  // The pthread struct has a field that points to itself.
   new->self = new;
+
+  // Thread ID, this becomes zero when the thread is no longer available.
   new->tid = next_tid++;
 
   // pthread struct robust_list head should point to itself.
@@ -177,13 +178,17 @@ void _emscripten_thread_free_data(pthread_t t) {
 #ifndef NDEBUG
   if (t->profilerBlock) {
     emscripten_builtin_free(t->profilerBlock);
+    t->profilerBlock = 0;
   }
 #endif
-  if (t->stack_owned) {
+  if (a_cas(&t->stack_owned, 1, 0)) {
     emscripten_builtin_free(((char*)t->stack) - t->stack_size);
   }
-  // To aid in debugging set all fields to zero
-  memset(t, 0, sizeof(*t));
+  // The tid may be reused. Clear it to prevent inadvertent use and inform functions
+  // that would use it that it's no longer available.
+  t->tid = 0;
+  // TODO(kleisauke): Should we set all fields to zero instead?:
+  //memset(t, 0, sizeof(*t));
   emscripten_builtin_free(t);
 }