node:worker_threads low-hanging fruit #18758
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rowing synchronously" This reverts commit 1448e6a.
…ference to GlobalObject
src/bun_js.zig
Outdated
| @@ -68,6 +68,7 @@ pub const Run = struct { | |||
| .args = ctx.args, | |||
| .graph = graph_ptr, | |||
| .is_main_thread = true, | |||
| .destruct_main_thread_on_exit = bun.getenvTruthy("BUN_DESTRUCT_VM_ON_EXIT"), | |||
There was a problem hiding this comment.
Suggested change
| .destruct_main_thread_on_exit = bun.getenvTruthy("BUN_DESTRUCT_VM_ON_EXIT"), | |
| .destruct_main_thread_on_exit = bun.getRuntimeFeatureFlag("BUN_DESTRUCT_VM_ON_EXIT"), |
Jarred-Sumner
requested changes
Apr 3, 2025
| auto* globalObject = defaultGlobalObject(lexicalGlobalObject); | ||
| if (globalObject && callback.isCallable()) { | ||
| if (auto* process = jsDynamicCast<Bun::Process*>(globalObject->processObject())) { | ||
| process->queueNextTick(globalObject, AsyncContextFrame::withAsyncContextIfNeeded(globalObject, callback)); |
There was a problem hiding this comment.
Can you check when Node calls this callback? It's unlikely called on process.nextTick
There was a problem hiding this comment.
They attach it to the message port to be called later. I didn't look very hard into where it gets called from there; I'll undo the change for now and investigate more for a later PR
There was a problem hiding this comment.
#18768 is where the fixed version of this will go
This reverts commits: - 92a73de Move napiEnvs to ScriptExecutionContext and give napi_env a strong reference to GlobalObject - a0e41dc Add test for #18139 - 806b59c Test napi_add_finalizer in main thread instead of worker - 110f398 Emit online event before Worker entrypoint runs - 6943c56 Accept callback in MessagePort.close These are being moved to new PRs.
This was referenced Apr 4, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Fixes some of the easier tests from test/parallel/test-worker-*
execArgv: []receives an empty array forprocess.execArgvinstead of inheriting the parent thread's array (I haven't found this covered in the Node tests yet so I added it to ours)Progress toward Bun crash with Worker #18139 (but not fixed; I may move that to another PR if I get a complete fix in)BUN_DESTRUCT_VM_ON_EXIT=1, which tells Bun to destroy the main thread's JavaScript VM when the process exits. Normally, Worker VMs are the only ones that get destroyed. The intent of this flag is that running a test or program with it will expose bugs that would otherwise only have occurred if the code was running inside a Worker that exited. For instance, the changes from Fix UDPSocket not being closed when VM destructs #18687 were motivated by running a Node UDP test withBUN_DESTRUCT_VM_ON_EXIT=1.node:fs/promisesFileHandleconstructor beFileHandleinstead ofFileHandle2, and adds a propertySymbol(messaging_transfer_symbol)(this is progress towardstest-worker-message-port-transfer-fake-js-transferable-internal.jsbut does not fix it just yet)postMessageavoid calling a user-overriddenArray.prototype[Symbol.iterator]autoSelectFamilyAttemptTimeoutDefaultin node:net unique per-thread. Our code doesn't use this value anywhere, but Node's test harness multiplies the value by a fixed amount before the tests run. This means that if you have a Node test that repeatedly spawns new Workers, each one will greatly increase the globalautoSelectFamilyAttemptTimeoutDefault, until eventually aTypeErroris thrown because the harness attempts to increase it past the u32 limit.worker.getHeapSnapshot()error if called on a Worker that isn't running or if the options object is invalidMakes theonlineevent fire right before a Worker starts running its entrypoint instead of right afterexitevent is fired with code 1 instead of 0 on a Worker which throws an uncaught exceptionworker.terminate, and emit a deprecation warning if you do soSymbolarguments passed in theargvandexecArgvarrays coerce toSymbol(description)instead of throwing an errorprocess.argvcontain[worker eval]instead of a Blob URL for Workers spawned witheval: trueenvoption spawning a Worker is not the right typeMessagePort, and makesMessagePort.closeaccept a callbackeval: trueWorkersTODOs:
terminatedoesn't fire theexitevent (this affects several tests)How did you verify your code works?
Node tests