node:worker_threads: improve error messages, support environmentData, emit worker event#18768
node:worker_threads: improve error messages, support environmentData, emit worker event#18768Jarred-Sumner merged 101 commits intomainfrom
Conversation
|
Updated 6:34 PM PT - May 8th, 2025
❌ @190n, your commit 90f6978 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 18768That installs a local version of the PR into your bun-18768 --bun |
…xpected exit code
| if (isEmpty()) { | ||
| RETURN_IF_EXCEPTION(throwScope, ); |
There was a problem hiding this comment.
| if (isEmpty()) { | |
| RETURN_IF_EXCEPTION(throwScope, ); | |
| auto is_empty = isEmpty(); | |
| RETURN_IF_EXCEPTION(throwScope, ); | |
| if (is_empty) { |
There was a problem hiding this comment.
are you suggesting to replace if (!isEmpty()) with if (!is_empty) too? couldn't the drainMicrotasks call cause isEmpty() to change?
src/bun.js/bindings/NodeURL.cpp
Outdated
| RETURN_IF_EXCEPTION(scope, {}); | ||
| auto domainToAsciiFunction = JSC::JSFunction::create(vm, globalObject, 1, "domainToAscii"_s, jsDomainToASCII, ImplementationVisibility::Public); | ||
| auto domainToUnicodeFunction = JSC::JSFunction::create(vm, globalObject, 1, "domainToUnicode"_s, jsDomainToUnicode, ImplementationVisibility::Public); | ||
| ASSERT(binding && domainToAsciiFunction && domainToUnicodeFunction); |
There was a problem hiding this comment.
would be easier to debug if this was individual asserts for each item. also is the binding one necessary with the addition of RETURN_IF_EXCEPTION?
There was a problem hiding this comment.
would be easier to debug if this was individual asserts for each item
done
also is the binding one necessary with the addition of RETURN_IF_EXCEPTION?
yes, binding should never be null at that point, but that's why an assertion is appropriate in case i'm wrong
|
Found an issue with environmentData so I need to fix that before this can merge |
nektro
left a comment
There was a problem hiding this comment.
test/js/sql/sql.test.ts is failing and test/js/node/test/parallel/test-worker-message-port-receive-message.js needs an updated message but LGTM after that
What does this PR do?
These are the changes from #18758 that caused regressions or needed deeper investigation to verify correctness:
Makes theonlineevent fire right before a Worker starts running its entrypoint instead of right afterMakesMessagePort.closeaccept a callback (currently we just pass the callback tonextTick, instead of calling it exactly when the port is really closed)Workerconstructor with the right error messagereceiveMessageOnPortgetEnvironmentData/setEnvironmentDataimplementationStops unrefing a worker after callingterminate()Makes terminating a worker throw the VM's termination exception (so thatterminate()can even interrupt code that blocks the event loop)threadIdof a Worker that's exited -1workerevent on the process whenever a Worker is createdHow did you verify your code works?
Added newly-passing Node tests and verify existing ones
regression todolist: