Skip to content

Conversation

@190n
Copy link
Contributor

@190n 190n commented Apr 4, 2025

What does this PR do?

These are the changes from #18758 that caused regressions or needed deeper investigation to verify correctness:

  • Makes the online event fire right before a Worker starts running its entrypoint instead of right after
    • reverted, since it was breaking too many other things to merge quickly + it didn't pass any more tests
  • Makes MessagePort.close accept a callback (currently we just pass the callback to nextTick, instead of calling it exactly when the port is really closed)
    • reverted, so that I can focus on regressions and merge this sooner -- I will redo this on another PR
  • Validates the first argument of the Worker constructor with the right error message
  • Fixes the error message when passing the wrong type to receiveMessageOnPort
  • Fixes the getEnvironmentData/setEnvironmentData implementation
  • Mentions the type of value in the error message when a transfer list contains a duplicated value
  • Stops unrefing a worker after calling terminate()
    • reverted, since it was breaking too many other things to merge quickly
  • Makes terminating a worker throw the VM's termination exception (so that terminate() can even interrupt code that blocks the event loop)
    • reverted, since it was breaking too many other things to merge quickly
  • Fixes exception handling in various places related to the termination exception
  • Makes a Worker terminated while synchronously running its entrypoint use exit code 1
  • Moves resolving the path of a Worker's entrypoint to the new thread so that errors are emitted as error events instead of thrown as exceptions
  • Makes Workers that are terminated before they start running any JS use exit code 0
  • Makes the threadId of a Worker that's exited -1
  • Emits the worker event on the process whenever a Worker is created

How did you verify your code works?

Added newly-passing Node tests and verify existing ones

regression todolist:

@robobun
Copy link
Collaborator

robobun commented Apr 4, 2025

Updated 6:34 PM PT - May 8th, 2025

@190n, your commit 90f6978 has 2 failures in Build #16321:


🧪   To try this PR locally:

bunx bun-pr 18768

That installs a local version of the PR into your bun-18768 executable, so you can run:

bun-18768 --bun

@190n 190n mentioned this pull request Apr 4, 2025
4 tasks
cirospaciari
cirospaciari previously approved these changes May 7, 2025
Comment on lines 81 to +82
if (isEmpty()) {
RETURN_IF_EXCEPTION(throwScope, );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (isEmpty()) {
RETURN_IF_EXCEPTION(throwScope, );
auto is_empty = isEmpty();
RETURN_IF_EXCEPTION(throwScope, );
if (is_empty) {

Copy link
Contributor Author

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?

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);
Copy link
Member

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?

Copy link
Contributor Author

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

@190n 190n requested a review from nektro May 8, 2025 17:51
@190n 190n marked this pull request as draft May 8, 2025 21:07
@190n
Copy link
Contributor Author

190n commented May 8, 2025

Found an issue with environmentData so I need to fix that before this can merge

@190n 190n marked this pull request as ready for review May 8, 2025 21:46
@190n 190n requested review from Jarred-Sumner and cirospaciari May 8, 2025 22:00
Jarred-Sumner
Jarred-Sumner previously approved these changes May 8, 2025
Copy link
Member

@nektro nektro left a 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

@190n 190n requested a review from nektro May 9, 2025 00:58
@Jarred-Sumner Jarred-Sumner merged commit b5f31a6 into main May 9, 2025
57 of 62 checks passed
@Jarred-Sumner Jarred-Sumner deleted the ben/worker-threads-2 branch May 9, 2025 02:27
Copy link
Member

@nektro nektro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants