Simplify thread clean / exit / terminate logic

Author: kleisauke

src/library_pthread.js

@@ -8,7 +8,7 @@ var LibraryPThread = {
   $PThread__postset: 'if (!ENVIRONMENT_IS_PTHREAD) PThread.initMainThreadBlock();',
   $PThread__deps: ['_emscripten_thread_init',
                    'emscripten_register_main_browser_thread_id',
-                   '$ERRNO_CODES', 'emscripten_futex_wake', '$killThread',
+                   '$ERRNO_CODES', 'emscripten_futex_wake',
                    '$cancelThread', '$cleanupThread'
 #if USE_ASAN || USE_LSAN
                    , '$withBuiltinMalloc'
@@ -191,7 +191,7 @@ var LibraryPThread = {
       Atomics.store(HEAPU32, (tb + {{{ C_STRUCTS.pthread.detach_state }}} ) >> 2, {{{ cDefine('DT_EXITED') }}}); // Mark the thread as no longer running.
 
       // Wake any joiner.
-      _emscripten_futex_wake(tb + {{{ C_STRUCTS.pthread.detach_state }}}, {{{ cDefine('INT_MAX') }}});
+      _emscripten_futex_wake(tb + {{{ C_STRUCTS.pthread.detach_state }}}, 1);
 
       // 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.
@@ -209,24 +209,24 @@ var LibraryPThread = {
         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.
-          postMessage({ 'cmd': 'exit' });
+          postMessage({ 'cmd': 'cleanupThread', 'thread': tb, 'terminate': false });
         }
       }
     },
 
+    terminateAllThreads__deps: ['$cleanupThread'],
     terminateAllThreads: function() {
       for (var t in PThread.pthreads) {
-        var pthread = PThread.pthreads[t];
-        if (pthread && pthread.worker) {
-          PThread.returnWorkerToPool(pthread.worker);
+        if (t && t.worker) {
+          cleanupThread(t, true);
         }
       }
       PThread.pthreads = {};
 
       for (var i = 0; i < PThread.unusedWorkers.length; ++i) {
         var worker = PThread.unusedWorkers[i];
 #if ASSERTIONS
-        assert(!worker.pthread); // This Worker should not be hosting a pthread at this time.
+        assert(!worker.pthread, 'This Worker should not be hosting a pthread at this time');
 #endif
         worker.terminate();
       }
@@ -238,14 +238,14 @@ var LibraryPThread = {
 #if ASSERTIONS
         assert(pthread, 'This Worker should have a pthread it is executing');
 #endif
-        PThread.freeThreadData(pthread);
-        worker.terminate();
+        cleanupThread(pthread, true);
       }
       PThread.runningWorkers = [];
     },
     freeThreadData: function(pthread) {
       if (!pthread) return;
       if (pthread.threadInfoStruct) {
+        // Free thread-specific data
         var tlsMemory = {{{ makeGetValue('pthread.threadInfoStruct', C_STRUCTS.pthread.tsd, 'i32') }}};
         {{{ makeSetValue('pthread.threadInfoStruct', C_STRUCTS.pthread.tsd, 0, 'i32') }}};
 #if PTHREADS_PROFILING
@@ -261,19 +261,23 @@ var LibraryPThread = {
       pthread.stackBase = 0;
       if (pthread.worker) pthread.worker.pthread = null;
     },
-    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.
+          PThread.unusedWorkers.push(worker);
+        }
         PThread.runningWorkers.splice(PThread.runningWorkers.indexOf(worker), 1); // Not a running Worker anymore
         PThread.freeThreadData(worker.pthread);
-        // Detach the worker from the pthread object, and return it to the worker pool as an unused worker.
+        // Detach the worker from the pthread object.
         worker.pthread = undefined;
       });
     },
@@ -345,9 +349,7 @@ var LibraryPThread = {
         } else if (cmd === 'spawnThread') {
           spawnThread(e.data);
         } 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') {
@@ -364,12 +366,6 @@ var LibraryPThread = {
           err('Thread ' + d['threadId'] + ': ' + d['text']);
         } else if (cmd === 'alert') {
           alert('Thread ' + d['threadId'] + ': ' + d['text']);
-        } else if (cmd === 'exit') {
-          // Detect whether the pthread_t structure is 'alive', it might have already been cleaned up
-          var alive = worker.pthread && Atomics.load(HEAPU32, (worker.pthread.threadInfoStruct + {{{ C_STRUCTS.pthread.self }}}) >> 2);
-          if (alive) {
-            PThread.returnWorkerToPool(worker);
-          }
         } else if (cmd === 'exitProcess') {
           // A pthread has requested to exit the whole application process (runtime).
 #if ASSERTIONS
@@ -478,41 +474,28 @@ var LibraryPThread = {
     }
   },
 
-  $killThread: function(pthread_ptr) {
-    if (ENVIRONMENT_IS_PTHREAD) throw 'Internal Error! killThread() can only ever be called from main application thread!';
-    if (!pthread_ptr) throw 'Internal Error! Null pthread_ptr in killThread!';
-    {{{ makeSetValue('pthread_ptr', C_STRUCTS.pthread.self, 0, 'i32') }}};
-    var pthread = PThread.pthreads[pthread_ptr];
-    pthread.worker.terminate();
-    PThread.freeThreadData(pthread);
-    // 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;
-  },
-
-  $cleanupThread: function(pthread_ptr) {
+  $cleanupThread: function(pthread_ptr, terminate) {
     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!';
     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:
+    // Check whether 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
+    //    S2: thread ends, 'cleanupThread' 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') }}};
+    //    MB: handles first 'cleanupThread' message, as thread is detached, so
+    //        returnWorkerToPool() is called and all thread related structs are freed/released.
+    //    MB: handles second 'cleanupThread' message which calls this function.
+    var alive = pthread && Atomics.exchange(HEAPU32, (pthread_ptr + {{{ C_STRUCTS.pthread.self }}} ) >> 2, 0);
+    if (alive) {
       var worker = pthread.worker;
-      PThread.returnWorkerToPool(worker);
+      PThread.returnWorkerToPool(worker, terminate);
     }
   },
 
@@ -768,9 +751,9 @@ var LibraryPThread = {
     // magic ID to detect whether the pthread_t structure is 'alive'.
     {{{ makeSetValue('threadInfoStruct', C_STRUCTS.pthread.self, 'threadInfoStruct', 'i32') }}};
 
-    // pthread struct prev and next should point to itself - this is used by
-    // pthread_key_delete for deleting thread-specific data.
-    {{{ makeSetValue('threadInfoStruct', C_STRUCTS.pthread.prev, 0, 'i32') }}};
+    // pthread struct prev and next should initially point to itself (see __init_tp in musl),
+    // this is used by pthread_key_delete for deleting thread-specific data.
+    {{{ makeSetValue('threadInfoStruct', C_STRUCTS.pthread.prev, 'threadInfoStruct', 'i32') }}};
     {{{ makeSetValue('threadInfoStruct', C_STRUCTS.pthread.next, 'threadInfoStruct', 'i32') }}};
 
     // pthread struct robust_list head should point to itself.
@@ -839,7 +822,7 @@ var LibraryPThread = {
   emscripten_builtin_pthread_join: 'pthread_join',
 #endif
 
-  pthread_kill__deps: ['$killThread', '$cancelThread', 'emscripten_main_browser_thread_id'],
+  pthread_kill__deps: ['$cancelThread', '$cleanupThread', 'emscripten_main_browser_thread_id'],
   pthread_kill: function(thread, signal) {
     if (signal < 0 || signal >= 65/*_NSIG*/) return ERRNO_CODES.EINVAL;
     if (thread === _emscripten_main_browser_thread_id()) {
@@ -860,8 +843,8 @@ var LibraryPThread = {
       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;
   },
@@ -1081,11 +1064,11 @@ var LibraryPThread = {
   },
 
   emscripten_cleanup_thread__asm: true,
-  emscripten_cleanup_thread__sig: 'vi',
+  emscripten_cleanup_thread__sig: 'vii',
   emscripten_cleanup_thread__deps: ['$cleanupThread'],
-  emscripten_cleanup_thread: function(thread) {
-    if (!ENVIRONMENT_IS_PTHREAD) cleanupThread(thread);
-    else postMessage({ 'cmd': 'cleanupThread', 'thread': thread });
+  emscripten_cleanup_thread: function(thread, terminate) {
+    if (!ENVIRONMENT_IS_PTHREAD) cleanupThread(thread, terminate);
+    else postMessage({ 'cmd': 'cleanupThread', 'thread': thread, 'terminate': terminate });
   },
 
   emscripten_conditional_set_current_thread_status_js: function(expectedStatus, newStatus) {

system/include/emscripten/threading.h

@@ -383,7 +383,7 @@ void emscripten_thread_sleep(double msecs);
 // Cleanups the pthread_t struct.
 // TODO(kleisauke): Improve description.
 // This is an internal function and generally not intended for user code.
-void emscripten_cleanup_thread(pthread_t thread);
+void emscripten_cleanup_thread(pthread_t thread, int terminate);
 
 #define EM_THREAD_STATUS int
 #define EM_THREAD_STATUS_NOTSTARTED 0

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

@@ -44,7 +44,7 @@ static int __pthread_timedjoin_np(pthread_t t, void **res, const struct timespec
 	__tl_sync(t);
 	if (res) *res = t->result;
 #ifdef __EMSCRIPTEN__ // XXX Emscripten cleanup thread
-	if (state == DT_EXITED) emscripten_cleanup_thread(t);
+	if (state == DT_EXITED) emscripten_cleanup_thread(t, 0);
 #else // XXX Emscripten map_base unused
 	if (t->map_base) __munmap(t->map_base, t->map_size);
 #endif