Conversation
The helper read the signal buffer and then cleared it as two separate operations. The buffer is shared with another thread, so a signal written between the read and the clear was overwritten and never delivered. Atomics.exchange() reads and clears it atomically. Add a test that a signal in a shared or non-shared buffer is delivered once and the buffer is left cleared. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All commit authors signed the Contributor License Agreement. |
|
@hoodmane @freakboy3742 when you have some spare time, could you guys take at quick look at this one? It's a really small change. Thank you! |
|
Code change looks good to me. I'm not sure about the test, does it actually fail without the fix? I don't understand how. We could potentially merge the change without the test. |
| int handling = Py_EMSCRIPTEN_SIGNAL_HANDLING; | ||
| emscripten_set_signal_buffer_js(signum, shared); | ||
| Py_EMSCRIPTEN_SIGNAL_HANDLING = 1; | ||
| _Py_CheckEmscriptenSignals(); |
There was a problem hiding this comment.
Maybe use CheckInterrupt() here since it should work and it's public api.
| self.addCleanup(signal.signal, signal.SIGUSR1, old_handler) | ||
|
|
||
| left_in_buffer = emscripten_check_signal_buffer(signal.SIGUSR1, shared) | ||
| for _ in range(1000): |
There was a problem hiding this comment.
Do we need this loop at all? It looks to me like the signal is triggered immediately by _Py_CheckEmscriptenSignals();. Also, emscripten_check_signal_buffer() turns signal handling back off so it certainly won't trigger later...
There was a problem hiding this comment.
I confess I just added the test because I was ashamed to contribute without one, but I also think it's not necessary. There's no way to reasonably test the thread stuff, so it's mostly a placeholder. I would be +1 for removing it.
There was a problem hiding this comment.
I think having some check that this mechanism at least sort of works is not a bad thing.
There was a problem hiding this comment.
Maybe add a comment that it's not a realistic test of the feature but it at least executes the code path to make sure it's not completely broken.
2 participants
Resolves #157548.
_Py_CheckEmscriptenSignals_Helperread the signal buffer and then cleared it, so a signal written by anotherthread between the two was lost.
Atomics.exchange()reads and clears it in one operation.Atomics.exchange()needs an integer typed array. That is what the buffer is in practice: Pyodide'sdocumentation creates a
Uint8Arrayover aSharedArrayBuffer, and workerd sets a plainUint8Array, whichAtomics.exchange()also accepts.The new test in
test_capi.test_emscriptenputs SIGUSR1 in a shared and in a non-sharedUint8Array, runsthe check once, and asserts that the handler ran and the buffer was left cleared. It cannot catch the race
itself, which needs a second thread writing at the wrong moment. That was checked with an Emscripten build of
this branch (Emscripten 6.0.9, Node.js 24.21.0): 5000 SIGUSR1s raised from another thread, each waiting for
the handler before the next, were all handled; with the previous helper a signal was lost within the first
~2000.
I used Claude to investigate the race, write the reproducers and prepare the change, and reviewed the result.