Skip to content
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

Rewrite "node:worker_thread" "Worker" creation #22783

Closed
bartlomieju opened this issue Mar 7, 2024 · 0 comments · Fixed by #22815
Closed

Rewrite "node:worker_thread" "Worker" creation #22783

bartlomieju opened this issue Mar 7, 2024 · 0 comments · Fixed by #22815
Assignees
Labels
node API polyfill Related to various "node:*" modules APIs node compat refactor

Comments

@bartlomieju
Copy link
Member

bartlomieju commented Mar 7, 2024

Right now using Worker from node:worker_thread is racy as the setup data (environmentData, threadId and workerData) arrive in the first message (#22672).

We need to rewrite how we initialize these workers. Currently they are piggy-backing off of Web Worker but that is not really correct. @satyarohith, @mmastrac and I had an offline discussion about it and we decide to rewrite large chunks of worker_threads module.

Here's what we need to do:

  1. Don't rely on Worker from 11_workers.js, instead use raw ops to create a worker instance in native code - this will lead to a bit of copy pasting of code, but that's fine for now. In essence we need to call op_create_worker there and have the code for Worker.#pollControl and Worker.#pollMessages() in ext/node/polyfills/worker_threads.ts.
  2. Once point 1 is addressed we need to extend op_create_worker to be able to accept data for environmentData (that needs to apply structured clone algorithm) and workerData (also using structured clone algo). This data should be passed to bootstrap.workerRuntime in runtime/web_worker.rs and then piped to internals.node.initialize() and further to internals.__initWorkerThreads().
  3. At this point we should be able to remove the racy message received from the host that sets up all the necessary data and proceed with further refactors.

EDIT: I was able to clean up this bit before addressing previous points (#22785)

  1. Once we're able to do that we should proceed to rewrite how isMainThread is handled in node:worker_threads - this is something that should be passed explicitly instead of relying on quirky PRIVATE_WORKER_THREAD_NAME.
  2. When we fix isMainThread we need to remove PRIVATE_WORKER_THREAD_NAME in favor of passing the name from option bag of worker_threads::Worker properly.
@bartlomieju bartlomieju added refactor node compat node API polyfill Related to various "node:*" modules APIs labels Mar 7, 2024
bartlomieju added a commit that referenced this issue Mar 8, 2024
This commit changes how we figure out if we're running on main
thread in `node:worker_threads` module. Instead of relying on quirky
"magic variable" for a name to check if we're on main thread, we are
now explicitly passing this information during bootstrapping of the
runtime. As a side effect, `WorkerOptions.name` is more useful
and matches what Node.js does more closely (though not fully).

Towards #22783
bartlomieju added a commit that referenced this issue Mar 11, 2024
This commit fixes race condition in "node:worker_threads" module were
the first message did a setup of "threadId", "workerData" and
"environmentData".
Now this data is passed explicitly during workers creation and is set up
before any user code is executed.

Closes #22783
Closes #22672

---------

Co-authored-by: Satya Rohith <me@satyarohith.com>
nathanwhit pushed a commit that referenced this issue Mar 14, 2024
This commit changes how we figure out if we're running on main
thread in `node:worker_threads` module. Instead of relying on quirky
"magic variable" for a name to check if we're on main thread, we are
now explicitly passing this information during bootstrapping of the
runtime. As a side effect, `WorkerOptions.name` is more useful
and matches what Node.js does more closely (though not fully).

Towards #22783
nathanwhit pushed a commit that referenced this issue Mar 14, 2024
This commit fixes race condition in "node:worker_threads" module were
the first message did a setup of "threadId", "workerData" and
"environmentData".
Now this data is passed explicitly during workers creation and is set up
before any user code is executed.

Closes #22783
Closes #22672

---------

Co-authored-by: Satya Rohith <me@satyarohith.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
node API polyfill Related to various "node:*" modules APIs node compat refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants