Fix multithreading to work in Deno and Bun #25947
Conversation
src/runtime_common.js
Outdated
| var parentPort = worker_threads['parentPort']; | ||
| parentPort.on('message', (msg) => global.onmessage?.({ data: msg })); | ||
| Object.assign(globalThis, { | ||
| postMessage: (msg) => parentPort['postMessage'](msg), |
There was a problem hiding this comment.
This can just be globalThis.postMessage now can't it?
And same for globalThis.self above?
(i.e. there is no point in using Object.assign for just one element, right?)
There was a problem hiding this comment.
Yeah, good idea. I've cleaned it up now in a few ways (direct assignments to globalThis properties, using globalThis consistently here instead of mixing with global (they're equal and global is deprecated), using const/let to make the variable local).
Macil
force-pushed
the
fixThreadingForDenoAndBun
branch
6 times, most recently
from
60733b1 to
b42b004
Compare
Macil
force-pushed
the
fixThreadingForDenoAndBun
branch
6 times, most recently
from
2c708ef to
f9c2ff3
Compare
Fixes denoland/deno#17171 Also adds several pthread tests with Bun and Deno
Macil
force-pushed
the
fixThreadingForDenoAndBun
branch
from
f9c2ff3 to
ab5c5c6
Compare
| return f; | ||
| } | ||
|
|
||
| const parentPort = worker_threads['parentPort']; |
There was a problem hiding this comment.
(The lines below were relying on this variable being set in runtime_common.js, which I had moved into an if-block and made not global. I discovered this after tests failed. I figured the simplest solution was to make this code use its own local variable instead of re-introducing the global variable.)
There was a problem hiding this comment.
To save on code size and keep this patch as small as possible can you revert this and hoist const parentPort = worker_threads['parentPort']; outside the if?
Also, we still use var in most places in our codebase sadly (there are a couple of reasons, that are probably not worth explaining here... i really should create an FAQ entry or some such for it).
|
|
||
| int main() { | ||
| EM_ASM({ Promise.reject("rejected!"); }); | ||
| EM_ASM({ Promise.reject(new Error("rejected!")); }); |
There was a problem hiding this comment.
(Node transforms uncaught promise rejections into Error objects, but Deno and Bun don't do this. The test this is used in would then fail in Deno and Bun because it expected to get an Error object. To fix the test, I just made it always create an Error object. This did require me to slightly update the expected output in test/test_browser.py for the browser versions of this test.)
|
I've added some pthread tests to the Bun tests and added some Deno tests too. (I originally added a few tests for Deno that happened to use Emscripten's |
| return config.DENO_ENGINE | ||
| return None | ||
|
|
||
| def get_node_bun_or_deno(self): |
There was a problem hiding this comment.
(I'm not sure if some increasingly specific helper functions like this were the best route to go. One alternative would have been to make the get/require_node helpers to allow Deno/Bun too because they're mostly interchangeable, but it's probably better to be explicit. Another alternative would've been to make some generic helpers that take named parameters declaring which of Node/Node-Canary/Deno/Bun are allowed.)
There was a problem hiding this comment.
I'm hoping we can just avoid this completely, but allowing NODE_JS_TEST to be set to /path/to/bun or /path/to/deno. No need for new config vars I think/hope.
sbc100
left a comment
sbc100
left a comment
There was a problem hiding this comment.
To keep this change small and focused can you remove all the Deno stuff and we can consider that in a separate PR.
| self.js_engines = [nodejs] | ||
| self.node_args += shared.node_pthread_flags(nodejs) | ||
| if nodejs: | ||
| self.node_args += shared.node_pthread_flags(engine) |
There was a problem hiding this comment.
I'm hoping we don't need to maybe any of these test changes, and deno and bun can both just pretent to be node.
| self: global, | ||
| postMessage: (msg) => parentPort['postMessage'](msg), | ||
| }); | ||
| globalThis.self = globalThis; |
There was a problem hiding this comment.
I noticed that code here was lacking a check for node. Added in: #25948
2 participants
Fixes denoland/deno#17171
Projects using Emscripten with pthreads don't work in Deno or Bun because those runtimes implement Web Worker APIs that Node doesn't, and Emscripten attempts to re-implement those APIs when running in Node/Deno/Bun in a way that conflicts with Deno and Bun's own implementations. This PR feature-detects Deno and Bun's
postMessageimplementation and avoids setting up its own conflicting implementation in that case.