Refactor and rename confusing pthreads async functions (#10772)

As noted in #9086, emscripten_async_queue_on_thread is a
misleading name since it is only async if calling another thread -
if we are already on the same thread, we just do the call
immediately.

Fixing that by making it actually async isn't an option, since
if the current thread never returns to the main event loop,
an async event would never happen. We have to fire it
synchronously in that case, but we don't know if that is the
case or not (it depends on user code). And it would be less
efficient for the common case.

As a solution emscripten_async_queue_on_thread has been renamed to
emscripten_dispatch_to_thread which no longer implies that it is async -
the operation is in fact only async if it is sent to another thread, while it
is sync if on the same one. A new emscripten_dispatch_to_thread_async
function is added which is always async.

That is a breaking change for people using that API. I would
guess not many have since while it is in threading.h it isn't
documented elsewhere. And this PR should help some of those
users (including the one that suggested this PR), so overall
it seems worth a breaking change.

Fixes #9086

ChangeLog.md

@@ -17,6 +17,11 @@ See docs/process.md for how version tagging works.
 
 Current Trunk
 -------------
+- `emscripten_async_queue_on_thread` has been renamed to
+  `emscripten_dispatch_to_thread` which no longer implies that it is async -
+  the operation is in fact only async if it is sent to another thread, while it
+  is sync if on the same one. A new `emscripten_dispatch_to_thread_async`
+  function is added which is always async.
 - Honor `CACHE` setting in config file as an alternative to `EM_CACHE`
   environment variable.
 - Remove `--cache` command line arg.  The `CACHE` config setting and the

src/deps_info.json

@@ -89,7 +89,8 @@
   "timegm": ["_get_tzname", "_get_daylight", "_get_timezone", "malloc"],
   "tzset": ["_get_tzname", "_get_daylight", "_get_timezone", "malloc"],
   "times": ["memset"],
-  "emscripten_set_canvas_element_size_calling_thread": ["emscripten_async_queue_on_thread_"],
+  "emscripten_set_canvas_element_size_calling_thread": ["_emscripten_call_on_thread"],
+  "emscripten_set_offscreencanvas_size_on_target_thread": ["_emscripten_call_on_thread"],
   "emscripten_websocket_new": ["malloc"],
   "emscripten_wget_data": ["malloc"],
   "emscripten_webgl_destroy_context": ["emscripten_webgl_make_context_current", "emscripten_webgl_get_current_context"],

src/library_html5.js

@@ -185,7 +185,7 @@ var LibraryJSEvents = {
       {{{ makeSetValue('varargs', 0, 'eventTypeId', 'i32') }}};
       {{{ makeSetValue('varargs', 4, 'eventData', 'i32') }}};
       {{{ makeSetValue('varargs', 8, 'userData', 'i32') }}};
-      _emscripten_async_queue_on_thread_(targetThread, {{{ cDefine('EM_FUNC_SIG_IIII') }}}, eventHandlerFunc, eventData, varargs);
+      __emscripten_call_on_thread(0, targetThread, {{{ cDefine('EM_FUNC_SIG_IIII') }}}, eventHandlerFunc, eventData, varargs);
       stackRestore(stackTop);
     },
 #endif
@@ -2878,7 +2878,7 @@ var LibraryJSEvents = {
     return {{{ cDefine('EMSCRIPTEN_RESULT_SUCCESS') }}};
   },
 
-  emscripten_set_offscreencanvas_size_on_target_thread_js__deps: ['$stringToNewUTF8'
+  emscripten_set_offscreencanvas_size_on_target_thread_js__deps: ['$stringToNewUTF8', '_emscripten_call_on_thread'
 #if MINIMAL_RUNTIME && !WASM_BACKEND
   , '$stackSave', '$stackAlloc', '$stackRestore'
 #endif
@@ -2897,7 +2897,7 @@ var LibraryJSEvents = {
     // these two threads will deadlock. At the moment, we'd like to consider that this kind of deadlock would be an Emscripten runtime bug, although if
     // emscripten_set_canvas_element_size() was documented to require running an event in the queue of thread that owns the OffscreenCanvas, then that might be ok.
     // (safer this way however)
-    _emscripten_async_queue_on_thread_(targetThread, {{{ cDefine('EM_PROXIED_RESIZE_OFFSCREENCANVAS') }}}, 0, targetCanvasPtr /* satellite data */, varargs);
+    __emscripten_call_on_thread(0, targetThread, {{{ cDefine('EM_PROXIED_RESIZE_OFFSCREENCANVAS') }}}, 0, targetCanvasPtr /* satellite data */, varargs);
     stackRestore(stackTop);
   },
 

system/include/emscripten/threading.h

@@ -332,10 +332,19 @@ EMSCRIPTEN_RESULT emscripten_wait_for_call_i(em_queued_call *call, double timeou
 
 void emscripten_async_waitable_close(em_queued_call *call);
 
-// Queues the given function call to be performed on the specified thread.
-void emscripten_async_queue_on_thread_(pthread_t target_thread, EM_FUNC_SIGNATURE sig, void *func_ptr, void *satellite, ...);
-
-#define emscripten_async_queue_on_thread(target_thread, sig, func_ptr, satellite, ...) emscripten_async_queue_on_thread_((target_thread), (sig), (void*)(func_ptr), (satellite),##__VA_ARGS__)
+int _emscripten_call_on_thread(int force_async, pthread_t target_thread, EM_FUNC_SIGNATURE sig, void *func_ptr, void *satellite, ...); // internal
+
+// Runs the given function on the specified thread. If we are currently on
+// that target thread then we just execute the call synchronously; otherwise it
+// is queued on that thread to execute asynchronously.
+// Returns 1 if it executed the code (i.e., it was on the target thread), and 0
+// otherwise.
+#define emscripten_dispatch_to_thread(target_thread, sig, func_ptr, satellite, ...) _emscripten_call_on_thread(0, (target_thread), (sig), (void*)(func_ptr), (satellite),##__VA_ARGS__)
+
+// Similar to emscripten_dispatch_to_thread, but always runs the
+// function asynchronously, even if on the same thread. This is less efficient
+// but may be simpler to reason about in some cases.
+#define emscripten_dispatch_to_thread_async(target_thread, sig, func_ptr, satellite, ...) _emscripten_call_on_thread(1, (target_thread), (sig), (void*)(func_ptr), (satellite),##__VA_ARGS__)
 
 // Returns 1 if the current thread is the thread that hosts the Emscripten runtime.
 int emscripten_is_main_runtime_thread(void);

system/lib/gl/webgl1.c

@@ -127,7 +127,7 @@ void glBufferData(GLenum target, GLsizeiptr size, const void *data, GLenum usage
       void *ptr = memdup(data, size);
       if (ptr || !data) // glBufferData(data=0) can always be handled asynchronously
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIII, &emscripten_glBufferData, ptr, target, size, ptr, usage);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIII, &emscripten_glBufferData, ptr, target, size, ptr, usage);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -149,7 +149,7 @@ void glBufferSubData(GLenum target, GLintptr offset, GLsizeiptr size, const void
       void *ptr = memdup(data, size);
       if (ptr || !data)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIII, &emscripten_glBufferSubData, ptr, target, offset, size, ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIII, &emscripten_glBufferSubData, ptr, target, offset, size, ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -291,7 +291,7 @@ void glTexImage2D(GLenum target, GLint level, GLint internalformat, GLsizei widt
       void *ptr = memdup(pixels, sz);
       if (ptr || !pixels)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIIIIIIII, &emscripten_glTexImage2D, ptr, target, level, internalformat, width, height, border, format, type, ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIIIIIIII, &emscripten_glTexImage2D, ptr, target, level, internalformat, width, height, border, format, type, ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -319,7 +319,7 @@ void glTexSubImage2D(GLenum target, GLint level, GLint xoffset, GLint yoffset, G
       void *ptr = memdup(pixels, sz);
       if (ptr || !pixels)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIIIIIIII, &emscripten_glTexSubImage2D, ptr, target, level, xoffset, yoffset, width, height, format, type, ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIIIIIIII, &emscripten_glTexSubImage2D, ptr, target, level, xoffset, yoffset, width, height, format, type, ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -344,7 +344,7 @@ void glUniform1fv(GLint location, GLsizei count, const GLfloat *value)
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform1fv, ptr, location, count, (GLfloat*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform1fv, ptr, location, count, (GLfloat*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -369,7 +369,7 @@ void glUniform1iv(GLint location, GLsizei count, const GLint *value)
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform1iv, ptr, location, count, (GLint*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform1iv, ptr, location, count, (GLint*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -393,7 +393,7 @@ void glUniform2fv(GLint location, GLsizei count, const GLfloat *value)
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform2fv, ptr, location, count, (GLfloat*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform2fv, ptr, location, count, (GLfloat*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -418,7 +418,7 @@ void glUniform2iv(GLint location, GLsizei count, const GLint *value)
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform2iv, ptr, location, count, (GLint*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform2iv, ptr, location, count, (GLint*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -442,7 +442,7 @@ void glUniform3fv(GLint location, GLsizei count, const GLfloat *value)
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform3fv, ptr, location, count, (GLfloat*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform3fv, ptr, location, count, (GLfloat*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -467,7 +467,7 @@ void glUniform3iv(GLint location, GLsizei count, const GLint *value)
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform3iv, ptr, location, count, (GLint*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform3iv, ptr, location, count, (GLint*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -491,7 +491,7 @@ void glUniform4fv(GLint location, GLsizei count, const GLfloat *value)
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform4fv, ptr, location, count, (GLfloat*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform4fv, ptr, location, count, (GLfloat*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -516,7 +516,7 @@ void glUniform4iv(GLint location, GLsizei count, const GLint *value)
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform4iv, ptr, location, count, (GLint*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIII, &emscripten_glUniform4iv, ptr, location, count, (GLint*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -539,7 +539,7 @@ void glUniformMatrix2fv(GLint location, GLsizei count, GLboolean transpose, cons
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIII, &emscripten_glUniformMatrix2fv, ptr, location, count, transpose, (GLfloat*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIII, &emscripten_glUniformMatrix2fv, ptr, location, count, transpose, (GLfloat*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -562,7 +562,7 @@ void glUniformMatrix3fv(GLint location, GLsizei count, GLboolean transpose, cons
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIII, &emscripten_glUniformMatrix3fv, ptr, location, count, transpose, (GLfloat*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIII, &emscripten_glUniformMatrix3fv, ptr, location, count, transpose, (GLfloat*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.
@@ -585,7 +585,7 @@ void glUniformMatrix4fv(GLint location, GLsizei count, GLboolean transpose, cons
       void *ptr = memdup(value, sz);
       if (ptr)
       {
-        emscripten_async_queue_on_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIII, &emscripten_glUniformMatrix4fv, ptr, location, count, transpose, (GLfloat*)ptr);
+        emscripten_dispatch_to_thread(*(void**)(pthread_getspecific(currentActiveWebGLContext) + 4), EM_FUNC_SIG_VIIII, &emscripten_glUniformMatrix4fv, ptr, location, count, transpose, (GLfloat*)ptr);
         return;
       }
       // Fall through on allocation failure and run synchronously.

system/lib/pthread/library_pthread.c

@@ -449,7 +449,7 @@ pthread_t EMSCRIPTEN_KEEPALIVE emscripten_main_browser_thread_id() {
   return main_browser_thread_id_;
 }
 
-static void EMSCRIPTEN_KEEPALIVE emscripten_async_queue_call_on_thread(
+int EMSCRIPTEN_KEEPALIVE do_emscripten_dispatch_to_thread(
   pthread_t target_thread, em_queued_call* call) {
   assert(call);
 
@@ -469,7 +469,7 @@ static void EMSCRIPTEN_KEEPALIVE emscripten_async_queue_call_on_thread(
   if (target_thread == EM_CALLBACK_THREAD_CONTEXT_CALLING_THREAD ||
       target_thread == pthread_self()) {
     _do_call(call);
-    return;
+    return 1;
   }
 
   // Add the operation to the call queue of the main runtime thread.
@@ -501,7 +501,7 @@ static void EMSCRIPTEN_KEEPALIVE emscripten_async_queue_call_on_thread(
       //message to thread. ' + new Error().stack));
       // #endif
       em_queued_call_free(call);
-      return;
+      return 0;
     }
   }
 
@@ -516,17 +516,19 @@ static void EMSCRIPTEN_KEEPALIVE emscripten_async_queue_call_on_thread(
     if (!success) {
       em_queued_call_free(call);
       pthread_mutex_unlock(&call_queue_lock);
-      return;
+      return 0;
     }
   }
 
   emscripten_atomic_store_u32((void*)&q->call_queue_tail, new_tail);
 
   pthread_mutex_unlock(&call_queue_lock);
+
+  return 0;
 }
 
 void EMSCRIPTEN_KEEPALIVE emscripten_async_run_in_main_thread(em_queued_call* call) {
-  emscripten_async_queue_call_on_thread(emscripten_main_browser_thread_id(), call);
+  do_emscripten_dispatch_to_thread(emscripten_main_browser_thread_id(), call);
 }
 
 void EMSCRIPTEN_KEEPALIVE emscripten_sync_run_in_main_thread(em_queued_call* call) {
@@ -846,13 +848,19 @@ em_queued_call* emscripten_async_waitable_run_in_main_runtime_thread_(
   return q;
 }
 
-void EMSCRIPTEN_KEEPALIVE emscripten_async_queue_on_thread_(
+int EMSCRIPTEN_KEEPALIVE _emscripten_call_on_thread(
+  int forceAsync,
   pthread_t targetThread, EM_FUNC_SIGNATURE sig, void* func_ptr, void* satellite, ...) {
   int numArguments = EM_FUNC_SIG_NUM_FUNC_ARGUMENTS(sig);
   em_queued_call* q = em_queued_call_malloc();
   assert(q);
+  // TODO: handle errors in a better way, this pattern appears in several places
+  //       in this file. The current behavior makes the calling thread hang as
+  //       it waits (for synchronous calls).
+  // If we failed to allocate, return 0 which means we did not execute anything
+  // (we also never will in that case).
   if (!q)
-    return;
+    return 0;
   q->functionEnum = sig;
   q->functionPtr = func_ptr;
   q->satelliteData = satellite;
@@ -882,8 +890,21 @@ void EMSCRIPTEN_KEEPALIVE emscripten_async_queue_on_thread_(
   // 'async' runs are fire and forget, where the caller detaches itself from the call object after
   // returning here, and it is the callee's responsibility to free up the memory after the call has
   // been performed.
+  // Note that the call here might not be async if on the same thread, but for
+  // consistency use the same convention of calleeDelete.
   q->calleeDelete = 1;
-  emscripten_async_queue_call_on_thread(targetThread, q);
+  // The called function will not be async if we are on the same thread; force
+  // async if the user asked for that.
+  if (forceAsync) {
+    EM_ASM({
+      setTimeout(function() {
+        _do_emscripten_dispatch_to_thread($0, $1);
+      }, 0);
+    }, targetThread, q);
+    return 0;
+  } else {
+    return do_emscripten_dispatch_to_thread(targetThread, q);
+  }
 }
 
 void llvm_memory_barrier() { emscripten_atomic_fence(); }

tests/html5_event_callback_in_two_threads.c

@@ -42,7 +42,7 @@ EM_BOOL keydown_callback_on_main_browser_thread(int eventType, const EmscriptenK
 #if __EMSCRIPTEN_PTHREADS__
   EmscriptenKeyboardEvent *duplicatedEventStruct = malloc(sizeof(*e));
   memcpy(duplicatedEventStruct, e, sizeof(*e));
-  emscripten_async_queue_on_thread(application_main_thread_id, EM_FUNC_SIG_IIII, keydown_callback_on_application_main_thread, duplicatedEventStruct, eventType, duplicatedEventStruct, userData);
+  emscripten_dispatch_to_thread(application_main_thread_id, EM_FUNC_SIG_IIII, keydown_callback_on_application_main_thread, duplicatedEventStruct, eventType, duplicatedEventStruct, userData);
 #else
   keydown_callback_on_application_main_thread(eventType, e, userData);
 #endif

tests/pthread/call_async.c

@@ -0,0 +1,35 @@
+/*
+ * Copyright 2020 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 <stdio.h>
+#include <assert.h>
+#include <emscripten/threading.h>
+
+int state = 0;
+
+void increment() {
+  state++;
+}
+
+void finish() {
+#ifdef REPORT_RESULT
+	REPORT_RESULT(1);
+#endif
+}
+
+int main() {
+  assert(state == 0);
+  // This dispatch_to_thread call will be synchronous since we are on the right
+  // thread already.
+  int called_now = emscripten_dispatch_to_thread(emscripten_main_browser_thread_id(), EM_FUNC_SIG_V, &increment, 0);
+  assert(called_now);
+  assert(state == 1);
+  // This async call will actually be async.
+  emscripten_dispatch_to_thread_async(emscripten_main_browser_thread_id(), EM_FUNC_SIG_V, &increment, 0);
+  assert(state == 1);
+  emscripten_dispatch_to_thread_async(emscripten_main_browser_thread_id(), EM_FUNC_SIG_V, &finish, 0);
+}