-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Fix multithreading to work in Deno and Bun #25947
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
base: main
Are you sure you want to change the base?
Conversation
sbc100
left a comment
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.
Nice!
We also now have some testing under bun as of #25903. Can you add 1 or more pthread-based tests to the lists of bun tests in .circleci/config.yml?
60733b1 to
b42b004
Compare
f9c2ff3 to
ab5c5c6
Compare
src/wasm_worker.js
Outdated
| return f; | ||
| } | ||
|
|
||
| const parentPort = worker_threads['parentPort']; |
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.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
|
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 |
test/common.py
Outdated
| return config.DENO_ENGINE | ||
| return None | ||
|
|
||
| def get_node_bun_or_deno(self): |
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.
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
I embraced that idea and it's much simpler now.
sbc100
left a comment
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.
To keep this change small and focused can you remove all the Deno stuff and we can consider that in a separate PR.
29a7b1f to
ff084dd
Compare
ff084dd to
5bd061c
Compare
tools/shared.py
Outdated
| if override: | ||
| actual = override | ||
| else: | ||
| actual = utils.run_process(nodejs + ['--version'], stdout=PIPE).stdout.strip() |
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.
Ideally this change would live only in test/ code and only effect NODE_JS_TEST, not the main version of node js that we use.
i.e. can you make local function in test/common.py that does this override? Then use the local version instead of shared.get_node_version?
sbc100
left a comment
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.
Nice!
lgtm % one last comment
4d052f0 to
071a233
Compare
Fixes denoland/deno#17171 Also adds several pthread tests with Bun
071a233 to
0b0e5ed
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.
Pull request overview
This PR fixes multithreading support for Deno and Bun runtimes by detecting their native Web Worker API implementations and avoiding conflicts with Emscripten's own reimplementation. The fix uses feature detection to check if postMessage already exists on the global scope, and only sets up Emscripten's Node.js-specific message handling when needed.
Key changes:
- Added feature detection for
globalThis.postMessagein pthread worker initialization code - Introduced test infrastructure to override Node.js version detection for non-Node runtimes
- Expanded CI testing to validate pthread functionality with Bun
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/runtime_common.js |
Conditionally sets up message handling only when postMessage is not already defined, allowing Deno/Bun's native implementation to be used |
src/pthread_esm_startup.mjs |
Applies the same postMessage detection logic for ESM integration mode |
test/common.py |
Adds get_node_test_version() helper to allow overriding version detection via OVERRIDE_NODE_JS_TEST_VERSION environment variable |
test/test_other.py |
Updates all version check call sites to use the new get_node_test_version() helper |
.circleci/config.yml |
Configures Bun testing with version override and adds pthread test coverage |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def get_node_test_version(self, nodejs): | ||
| override = os.environ.get('OVERRIDE_NODE_JS_TEST_VERSION') | ||
| if override: | ||
| override = override.removeprefix('v') | ||
| override = override.split('-')[0].split('.') | ||
| override = tuple(int(v) for v in override) | ||
| return override | ||
| return shared.get_node_version(nodejs) |
Copilot
AI
Dec 21, 2025
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.
Consider adding a docstring to document this helper function, especially the OVERRIDE_NODE_JS_TEST_VERSION environment variable behavior. This would help other developers understand when and why this override mechanism should be used. For example:
"""Get the Node.js version for testing purposes.
Returns the version tuple from OVERRIDE_NODE_JS_TEST_VERSION environment
variable if set, otherwise returns the actual version of the nodejs binary.
This allows testing with runtimes like Bun/Deno that masquerade as Node.js."""
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.Fixes: denoland/deno#17171