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.

Sign up for GitHub

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

Jump to bottom

worker: add postMessageToThread #53682

Merged
merged 1 commit into from
Jul 9, 2024
Merged

worker: add postMessageToThread #53682

merged 1 commit into from
Jul 9, 2024

Conversation

ShogunPanda
Copy link
Contributor

@ShogunPanda ShogunPanda commented Jul 2, 2024 •
edited
Loading

This PR is a total rethinking of #53488.

This PR adds a new API to worker_threads called postMessageToThread that allow to send arbitrary messages to any worker thread just identifying it by its threadId.

The recipient thread must have a listener on the new workerMessage event emitted on process otherwise the postMessageToThread will throw an exception.

In order not to have any thread block, the call is async to use Atomics.waitAsync.

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Jul 2, 2024
@ShogunPanda ShogunPanda mentioned this pull request Jul 2, 2024
Closed
mcollina
mcollina approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

I prefer this design

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 2, 2024
@targos targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 2, 2024
@ShogunPanda ShogunPanda added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jul 3, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 3, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
jasnell
jasnell reviewed Jul 4, 2024
View reviewed changes
doc/api/worker_threads.md Outdated Show resolved Hide resolved
jasnell
jasnell reviewed Jul 4, 2024
View reviewed changes
lib/internal/errors.js Outdated Show resolved Hide resolved
@Flarna Flarna added the worker Issues and PRs related to Worker support. label Jul 4, 2024
Flarna
Flarna reviewed Jul 4, 2024
View reviewed changes
doc/api/worker_threads.md Outdated Show resolved Hide resolved
Flarna
Flarna reviewed Jul 4, 2024
View reviewed changes
doc/api/worker_threads.md Outdated Show resolved Hide resolved
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 7, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
benjamingr
benjamingr approved these changes Jul 7, 2024
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

This is better

Flarna
Flarna approved these changes Jul 8, 2024
View reviewed changes
Copy link
Member

@Flarna Flarna left a comment

Choose a reason for hiding this comment

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

Not sure if new errors should be also marked as experimental like the API using it.

And I'm also not sure if workerMessage should be documented in process events as it is a reserved event now.

If we see a too high risk of clashes by reserving this event name we could move to a symbol.

@ShogunPanda
Copy link
Contributor Author

Not sure if new errors should be also marked as experimental like the API using it.

You are right: I marked them as well.

And I'm also not sure if workerMessage should be documented in process events as it is a reserved event now.

I documented so that users know they might have to listen to it.

If we see a too high risk of clashes by reserving this event name we could move to a symbol.

👍

@ShogunPanda ShogunPanda changed the title worker: add postMessageToWorker worker: add postMessageToThread Jul 8, 2024
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2024
@ShogunPanda ShogunPanda added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jul 8, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/60182/

@Flarna Flarna added process Issues and PRs related to the process subsystem. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Jul 9, 2024
@ShogunPanda ShogunPanda added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 9, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jul 9, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/53682
✔  Done loading data for nodejs/node/pull/53682
----------------------------------- PR info ------------------------------------
Title      worker: add postMessageToThread (#53682)
Author     Paolo Insogna <paolo@cowtech.it> (@ShogunPanda)
Branch     ShogunPanda:send-to-worker -> nodejs:main
Labels     semver-minor, process, lib / src, author ready, worker, needs-ci
Commits    1
 - worker: add postMessageToThread
Committers 1
 - Paolo Insogna <paolo@cowtech.it>
PR-URL: https://github.com/nodejs/node/pull/53682
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53682
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - worker: add postMessageToThread
   ℹ  This PR was created on Tue, 02 Jul 2024 08:38:46 GMT
   ✔  Approvals: 4
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/53682#pullrequestreview-2153146746
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/53682#pullrequestreview-2157902186
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/53682#pullrequestreview-2161833054
   ✔  - Gerhard Stöbich (@Flarna): https://github.com/nodejs/node/pull/53682#pullrequestreview-2162710662
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-07-08T19:33:46Z: https://ci.nodejs.org/job/node-test-pull-request/60182/
- Querying data for job/node-test-pull-request/60182/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/9852162608

@ShogunPanda ShogunPanda added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jul 9, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 9, 2024
@nodejs-github-bot nodejs-github-bot merged commit 66a635c into nodejs:main Jul 9, 2024
66 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 66a635c

@ShogunPanda ShogunPanda deleted the send-to-worker branch July 9, 2024 07:18
aduh95 pushed a commit that referenced this pull request Jul 12, 2024
@ShogunPanda @aduh95
worker: add postMessageToThread
180eef0 
PR-URL: #53682
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 added a commit that referenced this pull request Jul 12, 2024
@aduh95
2024-07-15, Version 22.5.0 (Current)
d4d03e2 
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
module:
  * add __esModule to require()'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
worker:
  * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682

PR-URL: TODO
@aduh95 aduh95 mentioned this pull request Jul 12, 2024
Merged
aduh95 added a commit that referenced this pull request Jul 12, 2024
@aduh95
2024-07-15, Version 22.5.0 (Current)
df75c77 
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
module:
  * add __esModule to require()'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
worker:
  * (SEMVER-MINOR) add postMessageToThread (Paolo Insogna) #53682

PR-URL: #53826
aduh95 pushed a commit that referenced this pull request Jul 16, 2024
@ShogunPanda @aduh95
worker: add postMessageToThread
22ca334 
PR-URL: #53682
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
aduh95 added a commit that referenced this pull request Jul 16, 2024
@aduh95
2024-07-16, Version 22.5.0 (Current)
b90d208 
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
aduh95 added a commit that referenced this pull request Jul 16, 2024
@aduh95
2024-07-16, Version 22.5.0 (Current)
3f9c06e 
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
aduh95 added a commit that referenced this pull request Jul 16, 2024
@aduh95
2024-07-16, Version 22.5.0 (Current)
433f759 
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
aduh95 added a commit that referenced this pull request Jul 17, 2024
@aduh95
2024-07-17, Version 22.5.0 (Current)
34de839 
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
RafaelGSS pushed a commit that referenced this pull request Jul 17, 2024
@aduh95 @RafaelGSS
2024-07-17, Version 22.5.0 (Current)
db1c2be 
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) #53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) #53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) #52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) #52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) #53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) #53462
test_runner:
  * support glob matching coverage files (Aviv Keller) #53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) #53682

PR-URL: #53826
ehsankhfr pushed a commit to ehsankhfr/node that referenced this pull request Jul 18, 2024
@aduh95 @ehsankhfr
2024-07-17, Version 22.5.0 (Current)
274b746 
Notable changes:

http:
  * (SEMVER-MINOR) expose websockets (Natalia Venditto) nodejs#53721
lib:
  * (SEMVER-MINOR) add `node:sqlite` module (Colin Ihrig) nodejs#53752
module:
  * add `__esModule` to `require()`'d ESM (Joyee Cheung) nodejs#52166
path:
  * (SEMVER-MINOR) add `matchesGlob` method (Aviv Keller) nodejs#52881
process:
  * (SEMVER-MINOR) port on-exit-leak-free to core (Vinicius Lourenço) nodejs#53239
stream:
  * (SEMVER-MINOR) pipeline wait for close before calling the callback (jakecastelli) nodejs#53462
test_runner:
  * support glob matching coverage files (Aviv Keller) nodejs#53553
worker:
  * (SEMVER-MINOR) add `postMessageToThread` (Paolo Insogna) nodejs#53682

PR-URL: nodejs#53826
@targos targos added the dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. label Sep 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@targos targos targos left review comments

@marco-ippolito marco-ippolito marco-ippolito approved these changes

@Flarna Flarna Flarna approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. process Issues and PRs related to the process subsystem. semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to Worker support.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@ShogunPanda @nodejs-github-bot @mcollina @jasnell @benjamingr @targos @Flarna @marco-ippolito