-
Notifications
You must be signed in to change notification settings - Fork 3.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix wasm workers under node #22721
Fix wasm workers under node #22721
Conversation
f53fb7a
to
c02abb5
Compare
9e79888
to
ac37d0b
Compare
ac37d0b
to
c9b2d8d
Compare
1eb97cb
to
d20e9e0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Though it is not precisely clear to me which part of this PR was the functional fix, and which part of the PR is nonfunctional/cleanup, since there are multiple orthogonal changes layered to this PR. (in particular, was refactoring to use callUserCallback()
a needed correctness fix for Node, and was removing use of dynCalls in test_wasm_worker_terminate test necessary for Node?)
I feel a bit that if I proposed this PR, it might get reviewed to need to be split to multiple parts with nonfunctional and functional changes separate 😅 - but since I do argue towards having fast development ergonomy and flow, I think it's fine.
I pretty sure everything here in the PR is essential to get the tests running under node. Sorry I should have documented in the PR description why each change is needed. I will do that now. |
d20e9e0
to
d5ce760
Compare
d5ce760
to
59d5100
Compare
We we not calling `writeStackCookie()` in the right place which means that `checkStackCookie()` would always fail in wasm workers. Required for #22721
59d5100
to
fb229d2
Compare
ecec63e
to
f17ce9c
Compare
I updated the PR description to detail why these changes are needed and split out some parts into cleanup PRs. |
f17ce9c
to
06855fd
Compare
06855fd
to
386bff2
Compare
386bff2
to
172855b
Compare
- Use callUserCallback to invoke callback in _wasmWorkerRunPostMessage. Without this calls to exit/emscripten_force_exit within the callback don't work as expected (they cause unhandled exception errors). - Fix `onmessage` handling under node so that the message payload always arrives as the `data` member of the message. - Update a few of the wasm workers tests do they actually exit (required for running tests under node).
172855b
to
8c1f739
Compare
Without this calls to exit/emscripten_force_exit within the callback
don't work as expected (they cause unhandled exception errors).
onmessage
handling under node so that the message payload alwaysarrives as the
data
member of the message.for running tests under node).