-
Notifications
You must be signed in to change notification settings - Fork 3.5k
node:worker_threads: improve error messages, support environmentData, emit worker event #18768
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (isEmpty()) { | |
| RETURN_IF_EXCEPTION(throwScope, ); | |
| auto is_empty = isEmpty(); | |
| RETURN_IF_EXCEPTION(throwScope, ); | |
| if (is_empty) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
nektro
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.
LGTM
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: