Remove incorrect/non-working (and used?) implementation of pthread_kill Our implementation of `pthread_kill` seems to be based on the assumption that this function is supposed to kill and given thread. This is not true. Instead `pthread_kill` works more like `kill` in that is delivers and async signal to a given thread. There are many such signals available such as SIGUSR1, SIGUSR2, SIGKILL, and SIGTERM. All that pthread_kill does that is different from `kill` is that it enables the caller to decided which thread the signal handler should run in. There is no signal that can sent that reults in a termination of a given thread. Sending SIGKILL (which is unblockable) will still bring down the entire program. There is no pthread API for asynronously killing a single thread, only pthread_cancel, which requires some cooperation from the thread being terminated. The currently implementation of `pthread_kill` is based on `worker.terminate()`, which will kill the worker even if it is in a busy loop (how does this work at the OS level I wonder?), is problematic becuse the thread has no way to clean up or free resources which can lead to memory leaks. Finally, `pthread_kill` does not currently work under chrome (brings down the entire tab) so this leads me to believe that it is very unlikely anyone is relying on the feature. Fixes: #14872
diff --git a/ChangeLog.md b/ChangeLog.md index 7d944e8..56e3e8b 100644 --- a/ChangeLog.md +++ b/ChangeLog.md
@@ -20,6 +20,8 @@ 2.0.27 ------ +- Remove implemenation of `pthread_kill` which did not work on chrome and + was incorrecly impemented. - Added `EM_ASYNC_JS` macro - similar to `EM_JS`, but allows using `await` inside the JS block and automatically integrates with Asyncify without the need for listing the declared function in `ASYNCIFY_IMPORTS` (#9709).
diff --git a/src/library_pthread.js b/src/library_pthread.js index deb8dca..309732f 100644 --- a/src/library_pthread.js +++ b/src/library_pthread.js
@@ -7,7 +7,7 @@ var LibraryPThread = { $PThread__postset: 'if (!ENVIRONMENT_IS_PTHREAD) PThread.initMainThreadBlock();', $PThread__deps: ['_emscripten_thread_init', - 'emscripten_futex_wake', '$killThread', + 'emscripten_futex_wake', '$cancelThread', '$cleanupThread', '$handleException', ], @@ -274,8 +274,6 @@ spawnThread(e.data); } else if (cmd === 'cleanupThread') { cleanupThread(d['thread']); - } else if (cmd === 'killThread') { - killThread(d['thread']); } else if (cmd === 'cancelThread') { cancelThread(d['thread']); } else if (cmd === 'loaded') { @@ -430,20 +428,6 @@ } }, - $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]; - delete 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) { 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!'; @@ -841,30 +825,6 @@ return __emscripten_do_pthread_join(thread, status, false); }, - pthread_kill__deps: ['$killThread', 'emscripten_main_browser_thread_id'], - pthread_kill: function(thread, signal) { - if (signal < 0 || signal >= 65/*_NSIG*/) return {{{ cDefine('EINVAL') }}}; - if (thread === _emscripten_main_browser_thread_id()) { - if (signal == 0) return 0; // signal == 0 is a no-op. - err('Main thread (id=' + thread + ') cannot be killed with pthread_kill!'); - return {{{ cDefine('ESRCH') }}}; - } - if (!thread) { - err('pthread_kill attempted on a null thread pointer!'); - return {{{ cDefine('ESRCH') }}}; - } - var self = {{{ makeGetValue('thread', C_STRUCTS.pthread.self, 'i32') }}}; - if (self !== thread) { - err('pthread_kill attempted on thread ' + thread + ', which does not point to a valid thread, or does not exist anymore!'); - return {{{ cDefine('ESRCH') }}}; - } - if (signal != 0) { - if (!ENVIRONMENT_IS_PTHREAD) killThread(thread); - else postMessage({ 'cmd': 'killThread', 'thread': thread}); - } - return 0; - }, - pthread_cancel__deps: ['$cancelThread', 'emscripten_main_browser_thread_id'], pthread_cancel: function(thread) { if (thread === _emscripten_main_browser_thread_id()) {
diff --git a/src/library_signals.js b/src/library_signals.js index eb23f48..ccb6aee 100644 --- a/src/library_signals.js +++ b/src/library_signals.js
@@ -37,6 +37,16 @@ #if ASSERTIONS err('Calling stub instead of kill()'); #endif + if (sig < 0 || sig >= 65/*_NSIG*/) return {{{ cDefine('EINVAL') }}}; + setErrNo({{{ cDefine('EPERM') }}}); + return -1; + }, + + pthread_kill: function(thread, sig) { +#if ASSERTIONS + err('Calling stub instead of pthread_kill()'); +#endif + if (sig < 0 || sig >= 65/*_NSIG*/) return {{{ cDefine('EINVAL') }}}; setErrNo({{{ cDefine('EPERM') }}}); return -1; },
diff --git a/tests/pthread/test_pthread_kill.cpp b/tests/pthread/test_pthread_kill.cpp deleted file mode 100644 index 86908bb..0000000 --- a/tests/pthread/test_pthread_kill.cpp +++ /dev/null
@@ -1,69 +0,0 @@ -// Copyright 2015 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 <signal.h> -#include <emscripten.h> - -_Atomic int sharedVar = 0; - -static void *thread_start(void *arg) -{ - // As long as this thread is running, keep the shared variable latched to nonzero value. - for(;;) - { - ++sharedVar; - } - - pthread_exit(0); -} - -pthread_t thr; - -void BusySleep(double msecs) -{ - double t0 = emscripten_get_now(); - while(emscripten_get_now() < t0 + msecs); -} - -int main() -{ - 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) - BusySleep(10); - - s = pthread_kill(thr, SIGKILL); - assert(s == 0); - - // Wait until we see the shared variable stop incrementing. (This is a bit heuristic and hacky) - for(;;) - { - int val = sharedVar; - BusySleep(100); - int val2 = sharedVar; - if (val == val2) break; - } - - // Reset to 0. - sharedVar = 0; - - // Wait for a long time, if the thread is still running, it should progress and set sharedVar by this time. - BusySleep(3000); - - // Finally test that the thread is not doing any work and it is dead. - assert(sharedVar == 0); - printf("Main: Done. Successfully killed thread. sharedVar: %d\n", sharedVar); - return 0; -}
diff --git a/tests/test_browser.py b/tests/test_browser.py index a4d52a8..d6b7cff 100644 --- a/tests/test_browser.py +++ b/tests/test_browser.py
@@ -3856,12 +3856,6 @@ def test_pthread_cancel_cond_wait(self): self.btest_exit(test_file('pthread/test_pthread_cancel_cond_wait.cpp'), assert_returncode=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 - def test_pthread_kill(self): - self.btest_exit(test_file('pthread/test_pthread_kill.cpp'), args=['-O3', '-s', 'USE_PTHREADS', '-s', 'PTHREAD_POOL_SIZE=8']) - # Test that pthread cleanup stack (pthread_cleanup_push/_pop) works. @requires_threads def test_pthread_cleanup(self):