Warn on blocking on the main thread (#9579)

kripken · web-flow · commit 3dc40117dbdf · 2019-10-31T15:13:55.000-07:00
Add a new ALLOW_BLOCKING_ON_MAIN_THREAD option, set to
1 by default. If unset, it will throw when blocking on the main thread.
If 1 as in the default case, we warn in the console about possible
issues and link to the docs about that.

This ignores things like mutexes which are usually brief, and checks
for pthread_join or pthread_cond_wait operations, which tend to be
longer and run the risk of Web-specific proxying-related deadlocks.

Add pthread_tryjoin_np which does a non-blocking join, which always
works on the main thread.

Improve the docs in this area.
diff --git a/ChangeLog.md b/ChangeLog.md
@@ -20,6 +20,10 @@ Current Trunk
  - `ERROR_ON_MISSING_LIBRARIES` now also applies to internal symbols that start
    with `emscripten_`.  Prior to this change such missing symbols would result
    in a runtime error, not they are reported at compile time.
+ - Pthread blocking on the main thread will now warn in the console. If
+   `ALLOW_BLOCKING_ON_MAIN_THREAD` is unset then the warning is an error.
+ - Add `pthread_tryjoin_np`, which is a POSIX API similar to `pthread_join`
+   but without blocking.
 
 v1.39.1: 10/30/2019
 -------------------
diff --git a/site/source/docs/porting/asyncify.rst b/site/source/docs/porting/asyncify.rst
@@ -1,4 +1,4 @@
-.. Asyncify:
+.. _Asyncify:
 
 ========================
 Asyncify
diff --git a/site/source/docs/porting/pthreads.rst b/site/source/docs/porting/pthreads.rst
@@ -43,17 +43,83 @@ but is unrelated. That flag does not use pthreads or SharedArrayBuffer, and
 instead uses a plain Web Worker to run your main program (and postMessage to
 proxy messages back and forth).
 
+Proxying
+========
+
+The Web allows certain operations to only happen from the main browser thread,
+like interacting with the DOM. As a result, various operations are proxied to
+the main browser thread if they are called on a background thread. See
+`bug 3495 <https://github.com/emscripten-core/emscripten/issues/3495>`_ for
+more information and how to try to work around this until then. To check which
+operations are proxied, you can look for the function's implementation in
+the JS library (``src/library_*``) and see if it is annotated with
+``__proxy: 'sync'`` or ``__proxy: 'async'``; however, note that the browser
+itself proxies certain things (like some GL operations), so there is no
+general way to be safe here (aside from not blocking on the main browser
+thread).
+
+In addition, Emscripten currently has a simple model of file I/O only happening
+on the main application thread (as we support JS plugin filesystems, which
+cannot share memory); this is another set of operations that are proxied.
+
+Proxying can cause problems in certain cases, see the section on blocking below.
+
+Blocking on the main browser thread
+===================================
+
+Note that in most cases the "main browser thread" is the same as the "main
+application thread". The main browser thread is where web pages start to run
+JavaScript, and where JavaScript can access the DOM (a page can also create a Web
+Worker, which would no longer be on the main thread). The main application
+thread is the one on which you started up the application (by loading the main
+JS file emitted by Emscripten). If you started it on the main browser thread -
+by it being a normal HTML page - then the two are identical. However, you can
+also start a multithreaded application in a worker; in that case the main
+application thread is that worker, and there is no access to the main browser
+thread.
+
+The Web API for atomics does not allow blocking on the main thread
+(specifically, ``Atomics.wait`` doesn't work there). Such blocking is
+necessary in APIs like ``pthread_join`` and anything that uses a futex wait
+under the hood, like ``usleep()``, ``emscripten_futex_wait()``, or
+``pthread_mutex_lock()``. To make them work, we use a busy-wait on the main
+browser thread, which can make the browser tab unresponsive, and also wastes
+power. (On a pthread, this isn't a problem as it runs in a Web Worker, where
+we don't need to busy-wait.)
+
+Busy-waiting on the main browser thread in general will work despite the
+downsides just mentioned, for things like waiting on a lightly-contended mutex.
+However, things like ``pthread_join`` and ``pthread_cond_wait``
+are often intended to block for long periods of time, and if that
+happens on the main browser thread, and while other threads expect it to
+respond, it can cause a surprising deadlock. That can happen because of
+proxying, see the previous section. If the main thread blocks while a worker
+attempts to proxy to it, a deadlock can occur.
+
+The bottom line is that on the Web it is bad for the main browser thread to
+wait on anything else. Therefore by default Emscripten warns if
+``pthread_join`` and ``pthread_cond_wait`` happen on the main browser thread,
+and will throw an error if ``ALLOW_BLOCKING_ON_MAIN_THREAD`` is zero
+(whose message will point to here).
+
+To avoid these problems, you can use ``PROXY_TO_PTHREAD``, which as
+mentioned earlier moves your ``main()`` function to a pthread, which leaves
+the main browser thread to focus only on receiving proxied events. This is
+recommended in general, but may take some porting work, if the application
+assumed ``main()`` was on the main browser thread.
+
+Another option is to replace blocking calls with nonblocking ones. For example
+you can replace ``pthread_join`` with ``pthread_tryjoin_np``. This may require
+your application to be refactored to use asynchronous events, perhaps through
+:c:func:`emscripten_set_main_loop` or :ref:`Asyncify`.
+
 Special considerations
 ======================
 
 The Emscripten implementation for the pthreads API should follow the POSIX standard closely, but some behavioral differences do exist:
 
 - When the linker flag ``-s PTHREAD_POOL_SIZE=<integer>`` is not specified and ``pthread_create()`` is called, the new thread will not start until control is yielded back to the browser's main event loop, because the web worker cannot be created while JS or wasm code is running. This is a violation of POSIX behavior and will break common code which creates a thread and immediately joins it or otherwise synchronously waits to observe an effect such as a memory write. Using a pool creates the web workers before main is called, allowing thread creation to be synchronous.
 
-- Browser DOM access is only possible on the main browser thread, and therefore things that may access the DOM, like filesystem operations (``fopen()``, etc.) or changing the HTML page's title, etc., are proxied over to the main browser thread. This proxying can generate a deadlock in a special situation that native code running pthreads does not have. See `bug 3495 <https://github.com/emscripten-core/emscripten/issues/3495>`_ for more information and how to work around this until this proxying is no longer needed in Emscripten. (To check which operations are proxied, you can look for the function's implementation in the JS library (``src/library_*``) and see if it is annotated with ``__proxy: 'sync'`` or ``__proxy: 'async'``.)
-
-- When doing a futex wait, e.g. ``usleep()``, ``emscripten_futex_wait()``, or ``pthread_mutex_lock()``, we use ``Atomics.wait`` on workers, which the browser should do pretty efficiently. But that is not available on the main thread, and so we busy-wait there. Busy-waiting is not recommended because it freezes the tab, and also wastes power.
-
 - The Emscripten implementation does not support `POSIX signals <http://man7.org/linux/man-pages/man7/signal.7.html>`_, which are sometimes used in conjunction with pthreads. This is because it is not possible to send signals to web workers and pre-empt their execution. The only exception to this is pthread_kill() which can be used as normal to forcibly terminate a running thread.
 
 - The Emscripten implementation does also not support multiprocessing via ``fork()`` and ``join()``.
diff --git a/src/library_pthread.js b/src/library_pthread.js
@@ -802,8 +802,18 @@ var LibraryPThread = {
     if (canceled == 2) throw 'Canceled!';
   },
 
-  {{{ USE_LSAN ? 'emscripten_builtin_' : '' }}}pthread_join__deps: ['_cleanup_thread', '_pthread_testcancel_js', 'emscripten_main_thread_process_queued_calls', 'emscripten_futex_wait'],
-  {{{ USE_LSAN ? 'emscripten_builtin_' : '' }}}pthread_join: function(thread, status) {
+  emscripten_check_blocking_allowed: function() {
+#if ASSERTIONS
+    assert(ENVIRONMENT_IS_WEB);
+    warnOnce('Blocking on the main thread is very dangerous, see https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread');
+#endif
+#if !ALLOW_BLOCKING_ON_MAIN_THREAD
+    abort('Blocking on the main thread is not allowed by default. See https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread');
+#endif
+  },
+
+  _emscripten_do_pthread_join__deps: ['_cleanup_thread', '_pthread_testcancel_js', 'emscripten_main_thread_process_queued_calls', 'emscripten_futex_wait', 'emscripten_check_blocking_allowed'],
+  _emscripten_do_pthread_join: function(thread, status, block) {
     if (!thread) {
       err('pthread_join attempted on a null thread pointer!');
       return ERRNO_CODES.ESRCH;
@@ -827,6 +837,11 @@ var LibraryPThread = {
       err('Attempted to join thread ' + thread + ', which was already detached!');
       return ERRNO_CODES.EINVAL; // The thread is already detached, can no longer join it!
     }
+
+    if (block && ENVIRONMENT_IS_WEB) {
+      _emscripten_check_blocking_allowed();
+    }
+
     for (;;) {
       var threadStatus = Atomics.load(HEAPU32, (thread + {{{ C_STRUCTS.pthread.threadStatus }}} ) >> 2);
       if (threadStatus == 1) { // Exited?
@@ -838,6 +853,9 @@ var LibraryPThread = {
         else postMessage({ 'cmd': 'cleanupThread', 'thread': thread });
         return 0;
       }
+      if (!block) {
+        return ERRNO_CODES.EBUSY;
+      }
       // TODO HACK! Replace the _js variant with just _pthread_testcancel:
       //_pthread_testcancel();
       __pthread_testcancel_js();
@@ -848,6 +866,16 @@ var LibraryPThread = {
     }
   },
 
+  {{{ USE_LSAN ? 'emscripten_builtin_' : '' }}}pthread_join__deps: ['_emscripten_do_pthread_join'],
+  {{{ USE_LSAN ? 'emscripten_builtin_' : '' }}}pthread_join: function(thread, status) {
+    return __emscripten_do_pthread_join(thread, status, true);
+  },
+
+  pthread_tryjoin_np__deps: ['_emscripten_do_pthread_join'],
+  pthread_tryjoin_np: function(thread, status) {
+    return __emscripten_do_pthread_join(thread, status, false);
+  },
+
   pthread_kill__deps: ['_kill_thread'],
   pthread_kill: function(thread, signal) {
     if (signal < 0 || signal >= 65/*_NSIG*/) return ERRNO_CODES.EINVAL;
diff --git a/src/settings.js b/src/settings.js
@@ -1325,6 +1325,14 @@ var PTHREAD_HINT_NUM_CORES = 4;
 // True when building with --threadprofiler
 var PTHREADS_PROFILING = 0;
 
+// It is dangerous to call pthread_join or pthread_cond_wait
+// on the main thread, as doing so can cause deadlocks on the Web (and also
+// it works using a busy-wait which is expensive). See
+// https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread
+// This may become set to 0 by default in the future; for now, this just
+// warns in the console.
+var ALLOW_BLOCKING_ON_MAIN_THREAD = 1;
+
 // If true, add in debug traces for diagnosing pthreads related issues.
 var PTHREADS_DEBUG = 0;
 
diff --git a/system/include/emscripten/threading.h b/system/include/emscripten/threading.h
@@ -372,6 +372,10 @@ struct thread_profiler_block
 	char name[32];
 };
 
+// Called when blocking on the main thread. This will error if main thread
+// blocking is not enabled, see ALLOW_BLOCKING_ON_MAIN_THREAD.
+void emscripten_check_blocking_allowed(void);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/system/lib/libc/musl/src/thread/pthread_cond_timedwait.c b/system/lib/libc/musl/src/thread/pthread_cond_timedwait.c
@@ -80,6 +80,12 @@ int __pthread_cond_timedwait(pthread_cond_t *restrict c, pthread_mutex_t *restri
 	int e, seq, clock = c->_c_clock, cs, shared=0, oldstate, tmp;
 	volatile int *fut;
 
+#ifdef __EMSCRIPTEN__
+	if (pthread_self() == emscripten_main_browser_thread_id()) {
+		emscripten_check_blocking_allowed();
+	}
+#endif
+
 	if ((m->_m_type&15) && (m->_m_lock&INT_MAX) != __pthread_self()->tid)
 		return EPERM;
 
diff --git a/tests/pthread/main_thread_join.cpp b/tests/pthread/main_thread_join.cpp
@@ -0,0 +1,87 @@
+// Copyright 2019 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 <assert.h>
+#include <emscripten.h>
+#include <pthread.h>
+#include <stdio.h>
+
+#include <atomic>
+
+pthread_t thread;
+
+std::atomic<int> tries;
+
+static const int EXPECTED_TRIES = 7;
+
+void loop() {
+  void* retval;
+  printf("try...\n");
+  if (pthread_tryjoin_np(thread, &retval) == 0) {
+    emscripten_cancel_main_loop();
+    assert(tries.load() == EXPECTED_TRIES);
+#ifdef REPORT_RESULT
+    REPORT_RESULT(2);
+#endif
+  }
+  tries++;
+}
+
+void *ThreadMain(void *arg) {
+#ifdef TRY_JOIN
+  // Delay to force the main thread to try and fail a few times before
+  // succeeding.
+  while (tries.load() < EXPECTED_TRIES) {}
+#endif
+	pthread_exit((void*)0);
+}
+
+pthread_t CreateThread() {
+  pthread_t ret;
+  int rc = pthread_create(&ret, NULL, ThreadMain, (void*)0);
+  assert(rc == 0);
+  return ret;
+}
+
+int main() {
+  if (!emscripten_has_threading_support()) {
+#ifdef REPORT_RESULT
+    REPORT_RESULT(0);
+#endif
+    printf("Skipped: Threading is not supported.\n");
+    return 0;
+  }
+
+  int x = EM_ASM_INT({
+    onerror = function(e) {
+      var message = e.toString();
+      var success = message.indexOf("Blocking on the main thread is not allowed by default. See https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread") >= 0;
+      if (success && !Module.reported) {
+        Module.reported = true;
+        console.log("reporting success");
+        // manually REPORT_RESULT; we shouldn't call back into native code at this point
+        var xhr = new XMLHttpRequest();
+        xhr.open("GET", "http://localhost:8888/report_result?0");
+        xhr.send();
+      }
+    };
+    return 0;
+  });
+
+  thread = CreateThread();
+#ifdef TRY_JOIN
+  emscripten_set_main_loop(loop, 0, 0);
+#else
+  int status;
+  // This should fail on the main thread.
+  puts("trying to block...");
+  pthread_join(thread, (void**)&status);
+  puts("blocked ok.");
+#ifdef REPORT_RESULT
+  REPORT_RESULT(1);
+#endif
+#endif // TRY_JOIN
+}
+
diff --git a/tests/pthread/main_thread_wait.cpp b/tests/pthread/main_thread_wait.cpp
@@ -0,0 +1,47 @@
+// Copyright 2019 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 <assert.h>
+#include <emscripten.h>
+#include <pthread.h>
+#include <stdio.h>
+
+int main() {
+  if (!emscripten_has_threading_support())
+  {
+#ifdef REPORT_RESULT
+    REPORT_RESULT(0);
+#endif
+    printf("Skipped: Threading is not supported.\n");
+    return 0;
+  }
+
+  int x = EM_ASM_INT({
+    onerror = function(e) {
+      var message = e.toString();
+      var success = message.indexOf("Blocking on the main thread is not allowed by default. See https://emscripten.org/docs/porting/pthreads.html#blocking-on-the-main-browser-thread") >= 0;
+      if (success && !Module.reported) {
+        Module.reported = true;
+        console.log("reporting success");
+        // manually REPORT_RESULT; we shouldn't call back into native code at this point
+        var xhr = new XMLHttpRequest();
+        xhr.open("GET", "http://localhost:8888/report_result?0");
+        xhr.send();
+      }
+    };
+    return 0;
+  });
+
+  // This should fail on the main thread.
+  puts("trying to block...");
+  pthread_cond_t cv = PTHREAD_COND_INITIALIZER;
+  pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
+  pthread_cond_wait(&cv, &lock);
+  puts("blocked ok.");
+#ifdef REPORT_RESULT
+  REPORT_RESULT(1);
+#endif
+}
+
diff --git a/tests/test_browser.py b/tests/test_browser.py
@@ -3620,6 +3620,24 @@ def test_pthread_64bit_cxx11_atomics(self):
       for pthreads in [[], ['-s', 'USE_PTHREADS=1']]:
         self.btest(path_from_root('tests', 'pthread', 'test_pthread_64bit_cxx11_atomics.cpp'), expected='0', args=opt + pthreads + ['-std=c++11'])
 
+  @parameterized({
+    'join': ('join',),
+    'wait': ('wait',),
+  })
+  @requires_threads
+  def test_pthread_main_thread_blocking(self, name):
+    print('Test that we error if not ALLOW_BLOCKING_ON_MAIN_THREAD')
+    self.btest(path_from_root('tests', 'pthread', 'main_thread_%s.cpp' % name), expected='0', args=['-O3', '-s', 'USE_PTHREADS=1', '-s', 'PTHREAD_POOL_SIZE=1', '-s', 'ALLOW_BLOCKING_ON_MAIN_THREAD=0'])
+    if name == 'join':
+      print('Test that by default we just warn about blocking on the main thread.')
+      self.btest(path_from_root('tests', 'pthread', 'main_thread_%s.cpp' % name), expected='1', args=['-O3', '-s', 'USE_PTHREADS=1', '-s', 'PTHREAD_POOL_SIZE=1'])
+      print('Test that tryjoin is fine, even if not ALLOW_BLOCKING_ON_MAIN_THREAD')
+      self.btest(path_from_root('tests', 'pthread', 'main_thread_join.cpp'), expected='2', args=['-O3', '-s', 'USE_PTHREADS=1', '-s', 'PTHREAD_POOL_SIZE=1', '-g', '-DTRY_JOIN', '-s', 'ALLOW_BLOCKING_ON_MAIN_THREAD=0'])
+      print('Test that tryjoin is fine, even if not ALLOW_BLOCKING_ON_MAIN_THREAD, and even without a pool')
+      self.btest(path_from_root('tests', 'pthread', 'main_thread_join.cpp'), expected='2', args=['-O3', '-s', 'USE_PTHREADS=1', '-g', '-DTRY_JOIN', '-s', 'ALLOW_BLOCKING_ON_MAIN_THREAD=0'])
+      print('Test that everything works ok when we are on a pthread.')
+      self.btest(path_from_root('tests', 'pthread', 'main_thread_%s.cpp' % name), expected='1', args=['-O3', '-s', 'USE_PTHREADS=1', '-s', 'PTHREAD_POOL_SIZE=1', '-s', 'PROXY_TO_PTHREAD', '-s', 'ALLOW_BLOCKING_ON_MAIN_THREAD=0'])
+
   # Test the old GCC atomic __sync_fetch_and_op builtin operations.
   @requires_threads
   def test_pthread_gcc_atomic_fetch_and_op(self):

Original file line number	Diff line number	Diff line change
`@@ -1,4 +1,4 @@`
`1`		`-.. Asyncify:`
	`1`	`+.. _Asyncify:`
`2`	`2`
`3`	`3`	`========================`
`4`	`4`	`Asyncify`