GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — workers: experimental BroadcastChannel by jasnell · Pull Request #36271 · nodejs/node

Conversation

@jasnell

@jasnell jasnell commented Nov 25, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Currently very experimental and definitely not finished implementation of BroadcastChannel for Node.js.

The intent for this is for it to be completely compatible with the browser API. It's not there yet.

For folks who are not familiar with BroadcastChannel, it is essentially a one-to-many alternative to MessageChannel.

'use strict';

const {
  isMainThread,
  BroadcastChannel,
  Worker
} = require('worker_threads');

const bc = new BroadcastChannel('hello');

if (isMainThread) {
  let c = 0;
  bc.onmessage = (event) => {
    console.log(event.data);
    if (++c === 10) bc.close();
  };
  for (let n = 0; n < 10; n++)
    new Worker(__filename);
} else {
  bc.postMessage('hello from every worker');
  bc.close();
}

All BroadcastChannel instances that share the same channel name, created in the main thread or worker threads, will receive a copy of the posted message.

Message delivery is at-most once, with no retention. When a message is posted, it is synchronously copied and queued into each of the other instances. Those other instances will deliver the message copy asynchronously.

Transferables are not supported with BroadcastChannel, so only messages that can be cloned are supported.

One use case of BroadcastChannel is to signal a group of worker threads that a process is shutting down, without having to send individual signals to each worker:

Worker:

const bc = new BroadcastChannel('pool-controller');
bc.onmessage = (({data}) => {
  if (data.shutdown)
    console.log('Shutting down!');
  bc.postMessage({done:true});
};

// Do worker stuff...

Main:

const bc = new BroadcastChannel('pool-controller');
bc.onmessage = ({data}) => console.log(data.done);
new Worker('worker.js');
setTimeout(() => bc.postMessage({shutdown:true}), 1000);

@addaleax: I'd be interested in your opinions on how to improve the C++ part of the implementation.
@benjamingr: I'd be interested in your thoughts on the JavaScript side of the implementation.

This is definitely a work in progress still, motivated by some needs around Piscina.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
devsnek, bricss, bajtos, rumkin, and xxoo reacted with thumbs up emoji bricss reacted with hooray emoji bricss, pc3b3r, and metcoder95 reacted with rocket emoji benjamingr, bricss, and metcoder95 reacted with eyes emoji
All reactions
  • 👍 5 reactions
  • 🎉 1 reaction
  • 🚀 3 reactions
  • 👀 3 reactions
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Nov 25, 2020
@jasnell
jasnell requested review from addaleax and benjamingr November 25, 2020 22:56
@jasnell jasnell added semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to the worker_threads module and Worker API. experimental Issues and PRs related to experimental features. labels Nov 25, 2020
addaleax
addaleax reviewed Nov 26, 2020
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

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.

I’ll have to think a bit about how this works on the C++ side … this definitely breaks the MessagePort/MessagePortData encapsulation in a big way, and probably in a way that can be avoided, and probably with less code than this would currently introduce…

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Nov 26, 2020

Copy link
Copy Markdown
Member

Basically, what I think I’d rather aim for is:

  • Making Messages deserializable multiple times, rather than just once
  • Instead of allowing only 1:1 sibling relationships on MessagePortData instances:
    • Introduce a SiblingGroup class (or similar name) that would extend to allowing more than 2 MessagePortData instances to be entangled with each other
    • Keeping a global [channel name] →SiblingGroup map so
  • Keep using regular MessagePorts instead of custom BroadcastPorts
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Nov 26, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

@jasnell Fwiw, if you want, I can probably find the time to write a prototype for that suggestion, and you can see if you like it or not :)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Nov 26, 2020

Copy link
Copy Markdown
Member Author

Thanks :-) I've already started working it up (yes it's a holiday here but this is Fun Coding not Work Coding so it's all good :-)...lol). The SiblingGroup is pretty straightforward. The one question I would have is what we should do if a Message that has transferables is dispatched to a SiblingGroup with more than one destination MessagePortData.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Nov 26, 2020

Copy link
Copy Markdown
Member

The one question I would have is what we should do if a Message that has transferables is dispatched to a SiblingGroup with more than one destination MessagePortData.

Yeah, I think there’s not much we can do besides fail … but we can do that during sending, right?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Nov 26, 2020

Copy link
Copy Markdown
Member Author

Yeah, I think there’s not much we can do besides fail … but we can do that during sending, right?

I think so. We can fail in PostMessage if the SiblingGroup has more than one destination port.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the broadcastchannel branch 2 times, most recently from 8a74b55 to 08ab8d6 Compare November 26, 2020 22:45
@jasnell

jasnell commented Nov 26, 2020

Copy link
Copy Markdown
Member Author

@addaleax ... ok, completely revised the implementation as discussed. Please take a look. Here, I'm done for the day and ready to go eat some turkey.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell mentioned this pull request Nov 26, 2020
ParentPort with multiple workers piscinajs/piscina#98
Closed
addaleax
addaleax reviewed Nov 26, 2020
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

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.

Yeah, I really like where this is going :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the broadcastchannel branch from 08ab8d6 to e31c009 Compare November 30, 2020 19:18
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 30, 2020
@jasnell
jasnell marked this pull request as ready for review November 30, 2020 19:20
@jasnell

jasnell commented Nov 30, 2020

Copy link
Copy Markdown
Member Author

@addaleax @benjamingr ... marking this ready for review now. There's still work that can be done on this to ensure that it's completely up to spec.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 30, 2020
@nodejs-github-bot

nodejs-github-bot commented Nov 30, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34637/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax requested changes Nov 30, 2020
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

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.

Looks good, apart from the error code thing :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/node_errors.h Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the broadcastchannel branch from e31c009 to d1c68a2 Compare November 30, 2020 23:07
@jasnell
jasnell requested a review from addaleax November 30, 2020 23:07
@jasnell

jasnell commented Nov 30, 2020

Copy link
Copy Markdown
Member Author

@addaleax .. updated! :-)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
addaleax
addaleax approved these changes Nov 30, 2020
View reviewed changes

@addaleax addaleax left a comment

Copy link
Copy Markdown
Member

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.

Nice :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/node_messaging.cc Resolved

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
jasnell force-pushed the broadcastchannel branch from d1c68a2 to 5e06220 Compare November 30, 2020 23:17
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 30, 2020
@jasnell jasnell added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Nov 30, 2020
@nodejs-github-bot

nodejs-github-bot commented Nov 30, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34638/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Nov 30, 2020
@nodejs-github-bot

nodejs-github-bot commented Nov 30, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/34639/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell
workers: add experimental BroadcastChannel
6066197 
Signed-off-by: James M Snell <[email protected]>
@jasnell
jasnell force-pushed the broadcastchannel branch from 94d2d6f to 6066197 Compare December 1, 2020 01:06
@jasnell jasnell mentioned this pull request Dec 1, 2020
Api allows workers to do initialization but it is not clear how to do cleanup piscinajs/piscina#87
Closed
benjamingr
benjamingr reviewed Dec 1, 2020
View reviewed changes
Comment thread doc/api/worker_threads.md

Closes the `BroadcastChannel` connection.

### `broadcastChannel.onmessage`

@benjamingr benjamingr Dec 1, 2020

Copy link
Copy Markdown
Member

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.

Any reason these are documented as event handlers and not event "message" etc?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@jasnell jasnell Dec 1, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

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.

Because from the documentation I've been able to find on web usage, the onmessage pattern tends to be more common. We can tweak the documentation later if necessary.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@benjamingr benjamingr Dec 1, 2020

Copy link
Copy Markdown
Member

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.

Just to clarify because of the "we can tweak the docs later" comment - a LGTM from me with comments always means "This is fine to land but I have these comments/questions we should probably address at some point".

If I think something isn't ready to land I never LGTM, I typically (like in this case) check the code out, play with it, put a debugger (in test/parallel/test-worker-broadcastchannel.js here, kind of annoying in worker_threads because I do it from ndb and not vscode), add some logs and LGTM.

I typically don't leave style nits because I usually really don't care about those.

There are a few (very minor) nits here that I might follow up with a PR about later if someone ever complains about them namely stuff like onmessage not being logged when you util.inspect a BroadcastChannel here but it being logged in Chrome. I honestly think these things aren't worth mentioning in PRs most of the time.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
benjamingr
benjamingr approved these changes Dec 1, 2020
View reviewed changes

@benjamingr benjamingr left a comment

Copy link
Copy Markdown
Member

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.

JS LGTM

The API itself leaves a lot to be desired but I guess we should follow the DOM's MessagePort so little we can do about it here :]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
jasnell added a commit that referenced this pull request Dec 1, 2020
@jasnell
worker: add experimental BroadcastChannel
9e446b3 
Signed-off-by: James M Snell <[email protected]>

PR-URL: #36271
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@jasnell

jasnell commented Dec 1, 2020

Copy link
Copy Markdown
Member Author

Landed in 9e446b3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Dec 1, 2020
addaleax added a commit to addaleax/node that referenced this pull request Dec 1, 2020
@addaleax
worker: refactor MessagePort entanglement management
6334542 
This addresses the `TODO` left on my request in 9e446b3. :)

Refs: nodejs#36271
@addaleax addaleax mentioned this pull request Dec 1, 2020
worker: refactor MessagePort entanglement management #36345
Closed
2 tasks done
@Trott

Trott commented Dec 2, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

This looks like it landed without a CI run after the rebase. Likely related: Windows CI now fails with the test added here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@Trott

Trott commented Dec 2, 2020

Copy link
Copy Markdown
Member

This looks like it landed without a CI run after the rebase. Likely related: Windows CI now fails with the test added here.

I don't have a Windows machine to test on locally, but I've optimistically opened #36353 changing the expected Windows behavior to be the spec-compliant behavior mentioned in the comment. If that passes CI, then I'll propose fast-tracking.
benjamingr reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
danielleadams pushed a commit that referenced this pull request Dec 7, 2020
@jasnell @danielleadams
worker: add experimental BroadcastChannel
802d44b 
Signed-off-by: James M Snell <[email protected]>

PR-URL: #36271
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
v15.4.0 proposal #36435
Merged
danielleadams added a commit that referenced this pull request Dec 7, 2020
@danielleadams
2020-12-08, Version 15.4.0 (Current)
fde09a4 
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
danielleadams added a commit that referenced this pull request Dec 9, 2020
@danielleadams
2020-12-09, Version 15.4.0 (Current)
5e5927e 
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
danielleadams added a commit that referenced this pull request Dec 9, 2020
@danielleadams
2020-12-09, Version 15.4.0 (Current)
348b7a9 
PR-URL: #36435

Notable changes:

* child_processes:
  * add AbortSignal support (Benjamin Gruenbaum) (#36308)
* deps:
  * update ICU to 68.1 (Michaël Zasso) (#36187)
* events:
  * support signal in EventTarget (Benjamin Gruenbaum) (#36258)
  * graduate Event, EventTarget, AbortController (James M Snell) (#35949)
* http:
  * enable call chaining with setHeader() (pooja d.p) (#35924)
* module:
  * add isPreloading indicator (James M Snell) (#36263)
* stream:
  * support abort signal (Benjamin Gruenbaum) (#36061)
  * add FileHandle support to Read/WriteStream (Momtchil Momtchev) (#35922)
* worker:
  * add experimental BroadcastChannel (James M Snell) (#36271)
nodejs-github-bot pushed a commit that referenced this pull request Dec 13, 2020
@addaleax @nodejs-github-bot
worker: refactor MessagePort entanglement management
88dfecc 
This addresses the `TODO` left on my request in 9e446b3. :)

Refs: #36271

PR-URL: #36345
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Dec 21, 2020
@addaleax @targos
worker: refactor MessagePort entanglement management
56fe9ba 
This addresses the `TODO` left on my request in 9e446b3. :)

Refs: #36271

PR-URL: #36345
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos added the backport-requested-v14.x label Aug 4, 2021
@targos

targos commented Aug 4, 2021

Copy link
Copy Markdown
Member

This will need a backport to land on v14.x
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@coderabbitai coderabbitai Bot mentioned this pull request Aug 12, 2026
fix(web): drain queued structured-view prompts while the chat is unmounted agent-of-empires/agent-of-empires#3332
Merged
7 of 24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@benjamingr benjamingr benjamingr approved these changes
@addaleax addaleax addaleax approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. experimental Issues and PRs related to experimental features. lib / src Issues and PRs involving general changes in the lib/ or src/ directories. semver-minor PRs that contain new features and should be released in the next minor version. worker Issues and PRs related to the worker_threads module and Worker API.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@jasnell @addaleax @nodejs-github-bot @Trott @targos @benjamingr
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.