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

workers: experimental BroadcastChannel #36271

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Nov 25, 2020

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

@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 related to general changes in the lib or src directory. labels Nov 25, 2020
@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 Worker support. experimental Issues and PRs related to experimental features. labels Nov 25, 2020
Copy link
Member

@addaleax addaleax left a 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…

src/node_messaging.cc Outdated Show resolved Hide resolved
src/node_messaging.cc Outdated Show resolved Hide resolved
src/node_messaging.h Outdated Show resolved Hide resolved
src/node_messaging.cc Outdated Show resolved Hide resolved
@addaleax
Copy link
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

@addaleax
Copy link
Member

addaleax commented Nov 26, 2020

@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 :)

@jasnell
Copy link
Member Author

jasnell commented Nov 26, 2020

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.

@addaleax
Copy link
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?

@jasnell
Copy link
Member Author

jasnell commented Nov 26, 2020

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.

@jasnell jasnell force-pushed the broadcastchannel branch 2 times, most recently from 8a74b55 to 08ab8d6 Compare November 26, 2020 22:45
@jasnell
Copy link
Member Author

jasnell commented Nov 26, 2020

@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.

Copy link
Member

@addaleax addaleax left a 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 :)

src/node_messaging.cc Outdated Show resolved Hide resolved
src/node_messaging.cc Outdated Show resolved Hide resolved
src/node_messaging.cc Show resolved Hide resolved
src/node_messaging.cc Outdated Show resolved Hide resolved
src/node_messaging.h Outdated Show resolved Hide resolved
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020
@jasnell jasnell marked this pull request as ready for review November 30, 2020 19:20
@jasnell
Copy link
Member Author

jasnell commented Nov 30, 2020

@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.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@addaleax addaleax left a 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 :)

src/node_errors.h Outdated Show resolved Hide resolved
src/node_messaging.cc Outdated Show resolved Hide resolved
src/node_messaging.cc Show resolved Hide resolved
src/node_messaging.cc Outdated Show resolved Hide resolved
src/node_messaging.cc Outdated Show resolved Hide resolved
@jasnell
Copy link
Member Author

jasnell commented Nov 30, 2020

@addaleax .. updated! :-)

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

Nice :)

src/node_messaging.cc Outdated Show resolved Hide resolved
@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020
@jasnell jasnell added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Nov 30, 2020
@nodejs-github-bot
Copy link
Collaborator

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 30, 2020
@nodejs-github-bot
Copy link
Collaborator

Signed-off-by: James M Snell <jasnell@gmail.com>

Closes the `BroadcastChannel` connection.

### `broadcastChannel.onmessage`
Copy link
Member

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?

Copy link
Member Author

@jasnell jasnell Dec 1, 2020

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.

Copy link
Member

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.

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.

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 :]

jasnell added a commit that referenced this pull request Dec 1, 2020
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36271
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Dec 1, 2020

Landed in 9e446b3

@jasnell jasnell closed this Dec 1, 2020
addaleax added a commit to addaleax/node that referenced this pull request Dec 1, 2020
This addresses the `TODO` left on my request in 9e446b3. :)

Refs: nodejs#36271
@Trott
Copy link
Member

Trott commented Dec 2, 2020

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

@Trott
Copy link
Member

Trott commented Dec 2, 2020

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.

danielleadams pushed a commit that referenced this pull request Dec 7, 2020
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: #36271
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@danielleadams danielleadams mentioned this pull request Dec 7, 2020
danielleadams added a commit that referenced this pull request Dec 7, 2020
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)
cjihrig pushed a commit to cjihrig/node that referenced this pull request Dec 8, 2020
Signed-off-by: James M Snell <jasnell@gmail.com>

PR-URL: nodejs#36271
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
danielleadams added a commit that referenced this pull request Dec 9, 2020
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
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
This addresses the `TODO` left on my request in 9e446b3. :)

Refs: #36271

PR-URL: #36345
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
targos pushed a commit that referenced this pull request Dec 21, 2020
This addresses the `TODO` left on my request in 9e446b3. :)

Refs: #36271

PR-URL: #36345
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@targos
Copy link
Member

targos commented Aug 4, 2021

This will need a backport to land on v14.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. 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 related to general changes in the lib or src directory. 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
Development

Successfully merging this pull request may close these issues.

6 participants