[clementperon]

em_task_queue_cancel runs cancel handlers with the queue lock held. A handler can block (cancel_ctx proxies back to the sender), and a blocked thread processes its dlopen queue from _emscripten_yield. When that is the queue being cancelled, a thread exiting during dlopen() deadlocks on its own lock:

_emscripten_thread_exit
  em_task_queue_cancel (thread mailbox)
    cancel_notification
      em_task_queue_cancel (dlopen queue, lock held)
        cancel_ctx -> do_proxy -> contended lock -> futex_wait
          _emscripten_yield -> _emscripten_process_dlopen_queue
            em_task_queue_execute (same queue) -> "pthread mutex deadlock detected"

Related to #18887. em_task_queue_execute already releases the lock around each task; this does the same for cancel handlers, and marks the queue as processing during cancellation so a re-entrant em_task_queue_execute cannot run the remaining tasks instead of cancelling them.

test_pthread_dlopen_exit keeps several threads creating and exiting threads while main dlopen()s 100 side modules. 100 runs each:

	Without fix	With fix
-O0	27 assert ("pthread mutex deadlock detected")	100 pass
-O2	8 hang	100 pass

[gemini-code-assist]

Code Review

This pull request updates em_task_queue_cancel in system/lib/pthread/em_task_queue.c to unlock the queue's mutex before executing the task's cancel handler and lock it again afterward, preventing potential deadlocks if the handler blocks. There are no review comments, so I have no feedback to provide.

[clementperon]

@sbc100
Stress-tested on 6.0.9: a host pthread dlopen()s side modules while threads are created and exit, and the side module starts its own threads.

Release build:

• 15/300 runs hang or abort with "pthread mutex deadlock detected" without this change
• 0/600 with it.

Build with -fsanitize=address -sASSERTIONS=2

• 7/100 runs abort without it ("attempt to load … a second time, before the first load completed")
• 0/100 with it and no AddressSanitizer reports.

[sbc100]

Thanks @clementperon for tracking down this flakiness and fixing it.

@tlively does the fix looks reasonable?

[clementperon]

@tlively as a side note during my testing I also reach another issue and when I try to open a PR it was similar to this one #27846

[tlively]

Seems good directionally, but Gemini found a problem. If there is a re-entrant em_task_queue_execute while the tasks are being canceled, then it will end up executing rather than canceling the remaining tasks. Gemini suggests we set queue->processing = 1 for the duration of em_task_queue_cancel, mirroring how it works in em_task_queue_execute.

Gemini suggests this additional test for that problem (edit: new test that does not depend on proxying queue internals):

#include <assert.h>
#include <dlfcn.h>
#include <pthread.h>
#include <stdbool.h>
#include <stdio.h>

#define NUM_ITERATIONS 100

static pthread_barrier_t barrier;
static _Atomic bool stop = false;

// Worker thread that repeatedly starts and immediately exits while dlopen()s occur
static void* exiting_worker(void* arg) {
  pthread_barrier_wait(&barrier);
  // Thread exits immediately while dynamic link synchronization calls are in-flight
  return NULL;
}

// Loader thread that continuously dlopen()s libraries to generate pending dlsync tasks
static void* loader_thread(void* arg) {
  pthread_barrier_wait(&barrier);
  while (!stop) {
    void* handle = dlopen("libside.so", RTLD_NOW | RTLD_GLOBAL);
    assert(handle && "dlopen failed");
    dlclose(handle);
  }
  return NULL;
}

int main() {
  pthread_barrier_init(&barrier, NULL, 2);

  pthread_t loader;
  pthread_create(&loader, NULL, loader_thread, NULL);

  for (int i = 0; i < NUM_ITERATIONS; ++i) {
    pthread_t worker;
    pthread_create(&worker, NULL, exiting_worker, NULL);
    pthread_join(worker, NULL);
  }

  stop = true;
  pthread_join(loader, NULL);
  pthread_barrier_destroy(&barrier);

  printf("PASSED\n");
  return 0;
}

Side module (libside.c):

int side_func() {
  return 42;
}

AI explanation of the test (I haven't vetted it):

1. The loader thread continuously calls dlopen(). For each load, _emscripten_dlsync_threads() proxies a synchronization task to all active threads on _dlopen_proxying_queue.
2. The worker thread spawns and immediately exits (pthread_exit).
3. While the worker thread is exiting, multiple dlsync tasks are queued for it.
4. The worker enters _emscripten_thread_mailbox_shutdown(), which runs em_task_queue_cancel() on _dlopen_proxying_queue.
5. The cancel handler cancel_ctx attempts to proxy back to the loader thread, which is currently holding the dynamic link lock inside dlopen().
6. The worker contends on the lock and calls emscripten_futex_wait() $\to$ _emscripten_yield() $\to$ _emscripten_process_dlopen_queue().
7. Without PR Release the task queue lock while running cancel handlers #28047:
   The worker tries to acquire queue->mutex while already holding it $\to$ aborts with "pthread mutex deadlock detected" (the flakiness in test_pthread_dlopen_many is very flaky #18887).
8. Under PR Release the task queue lock while running cancel handlers #28047 (mutex unlocked, but queue->processing == 0):
   The worker's yield hook re-entrantly acquires queue->mutex and executes the remaining do_thread_sync tasks on the dying thread rather than cancelling them.
9. With queue->processing = 1:
   emscripten_proxy_execute_queue() sees tasks->processing == 1 and skips execution. All tasks are cleanly cancelled, and the test runs to completion reliably.

[clementperon]

I deep dive into this and indeed it can happen, seems that a blocking cancellation handler can re-enter the dlopen queue and execute the remaining task.
I’ve added queue->processing = 1 during cancellation and reset it afterward.

[tlively]

Can you add the test as well?

[clementperon]

@tlively fine for you ?

[rustyconover]

@tlively is anything still needed here? We have a change that depends on this one (#28176), so here's some extra data.

With #28176's parallel sync on main, on aarch64 Linux with Node 24.19.0, a stress test where a thread exits during a dlopen sync hung 73 of 1,500 runs without this PR and 0 of 3,000 with it.

One caveat: test_pthread_dlopen_exit didn't reproduce the deadlock for me on unmodified main at -O2 under node: 600 of 600 runs passed. So my numbers support the fix, but only with #28176 applied, and I can't confirm the test's failure rate without this PR.

The codesize expectations in this PR also conflict with current main, so those need a refresh.

[tlively]

Thanks for the ping. Yes, this LGTM.

[tlively]

(We can land it after the code size tests are updated)

[clementperon]

@rustyconover I confirm that this test rarely trigger the issue I got a hit around 1/1600. So it's too weak to be reliable. New one is ~29/100

@sbc100 I have push your remarks but I had to force push to rewrite some of my emails. Sorry about that.

[clementperon]

@sbc100 about moving the test to stress_test, I'd rather keep it in test_core. The new version reproduces the deadlock in a single run (~27% at -O0), so it doesn't need the stress harness, and test_core runs it in every configuration on each CI run. It also relies on the dylink helpers (build_dlfcn_lib/prep_dlfcn_main) that only exist in test_core, and it sits alongside the related test_pthread_dlopen* tests there.

[sbc100]

processing is documented as only accessed on the target thread. I assume that em_task_queue_cancel is only ever called the target thread?

Should we assert(pthread_self() == queue->thread) here? Or is that overkill?