Fix a rare pthreads main thread deadlock that worsened in 2.0.2 #12318
Conversation
system/lib/pthread/library_pthread.c
Outdated
| // Stores the memory address that the main thread is waiting on, if any. If | ||
| // the main thread is waiting, we wake it up before waking up any workers. | ||
| EMSCRIPTEN_KEEPALIVE | ||
| int main_thread_futex; |
There was a problem hiding this comment.
Why int if this stores a memory address? Or is it the address of this data that is useful?
There was a problem hiding this comment.
Oh you're right, this should be a void*, fixed.
While in general futexes are always 32 bits, this is not an actual futex, it's the address of the futex (which some day soon may be 64 bits).
|
@juj please review this when you can. Landing now as the testcase shows this resolves an issue, and it may fix some other user reports we are getting. |
|
(also @sbc100 the question at the end of the first comment, again, no rush) |
There was a problem hiding this comment.
I believe there's a race here when calling the initShared() call in initWorker(). The worker JS can init prior to the module being fully loaded (due to workers js being loaded in parallel to the main js) - at that point we don't yet have the main thread futex. It looks as if initShared is idempotent; the main_thread_futex shouldn't change - could you always call it as the first line in worker.js, command == run?
|
@belraquib Hmm, I think it's safe already, but I may be wrong. We call |
|
Looking at the generated code for the worker, i see the call to Could this be an ordering issue in the jsifier? |
|
Ah, closing this, #12308 fixes it (and I only had this change patched into 2.0.2). |
This variable is initialized from `_main_thread_futex` which is a data address exported from native code. Rather than copy it we can simply use that variable directly. I think it made sense to have a seperate JS variable in the past when this address was allocated at runtime, but since #12318 landed it is no longer needed.
This variable is initialized from `_main_thread_futex` which is a data address exported from native code. Rather than copy it we can simply use that variable directly. I think it made sense to have a separate JS variable in the past when this address was allocated at runtime, but since #12318 landed it is no longer needed. Also rename the exported variable to avoid conflict with user symbols.
The deadlock was introduced in #12055 which was released in 2.0.2. However, the
issue is deeper as the deadlock was just made more frequent by that.
Background: Our pthreads implementation has some extra complexity because of
how the main thread works on the web, specifically, we can't block there. So we
can't do
Atomics.waitthere. Instead, we do a busy-wait in effect, and to make thatas least bad as possible, we special case the main thread. There is a memory address,
the "main thread futex address", which indicates the futex the main thread is waiting
on, or 0 if none. When a thread runs
futex_wake, it first checks if the main threadis waiting on that futex by checking that address, and if it is, we notify it first before
any workers. In effect this gives the main thread priority. And by using that address,
the main thread can busy-wait on it, just polling to see when a pthread sets it to 0,
which indicates the wait can stop.
This broke in #12055 because it erroneously ended up setting the main thread
futex address only on the main thread. We did not get that address to workers.
Instead, they got a JS
undefined. So they would not wake up the main thread.It is actually a lot more complicated than that, because
futex_waitcan recurse:while it does the busy-wait loop, it will run the event loop, so it does not starve
pthreads and get deadlocks - that is, it checks if there are incoming proxied
calls, and it runs them. To run the event loop we must take a mutex. With the
right timing we can hit the slow paths in both the original mutex (leading to
a call to
futex_waitinstead of a quick atomic operation) and in the proxyingmutex, and then we get this recursion. This was not handled by the old code
correctly, in two ways.
First, it did not clear the main thread futex address when
futex_waitexitsdue to a timeout. So it looked like we were still waiting. A later
futex_wakeon that futex would unset the main thread futex address, counting that as
one of the things it wakes up - but nothing was woken up there, so it ended
up not waking anyone up (if it was told to just wake 1 person).
Second,
futex_wakedid not check if it was on the main thread. The mainthread should never wake itself up, but it did, making it more confusing
(I'm not sure if this caused deadlocks by itself, though).
A possible third issue is that when recursing in that way we could have
the main thread in effect waiting on two futexes, A and then B. If a
worker wants to wake up A while in the recursed call that set B, we
don't see A in the main thread futex address - there is just one such
address, and B has trampled it. The old code would actually just
stop waiting in this case, since it looped while the address is the
address we set, which means that after it called the nested wait,
which wrote B, when we return to the first loop we'd see 0 (if the wait
for B ended cleanly) or B (if we timed out and due the bug mentioned
earlier we left B as the main thread futex address erroneously). And
both are not A so we'd stop. That wasn't correct, though - it's possible
we should still be waiting for A.
To fix all this, this PR does two things:
is accessible in workers properly.
futex_waitworks: it sets the main thread futexaddress and unsets it before exiting. It also unsets it before
doing a nested call, so that they cannot interfere with each other.
This also adds a lot of assertions in that code, which would have caught
the previous bug, and I also verified happen on our test suite, so we have
decent coverage.
The one tricky thing here is handling the case in the paragraph from
earlier with futexes A and B that we wait for in a nested manner. When
nested, we wait until B is done, then return to the loop for A. How can
we tell if A is done or not? The pthread waking it up may have already
had its chance to wake it. The solution taken here is to read the futex's
memory value, exactly like when starting in
futex_wait. That is,futex_waitchecks if an address has a value, and only if it does thenit waits. I believe it is valid to check it later as well, since the result
is the same as if the main thread were busy with other stuff, then
checked the value later, after the other thread did some work. See
the comment in the source for more details.
This fixes a test that was wrong in how it handled a race condition,
and adds a disabled test that was very useful for manual debugging
here. That test hammers on all the proxying and locking mechanisms
at once, basically. It's not a small little test, but I couldn't find a way
to simplify it, and it's just for manual debugging.
@sbc100 I did a KEEPALIVE on a global in C, and it seems to just work, leading to
something like
Module['_main_thread_futex'] = 4060;in the output, which is nice,but I was a little surprised it works - is it ok to depend on that?