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

@addaleax addaleax left a comment

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

@addaleax addaleax commented Nov 26, 2020

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

@addaleax addaleax commented Nov 26, 2020

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 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 jasnell:broadcastchannel branch 2 times, most recently Nov 26, 2020
@jasnell
Copy link
Member Author

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

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 force-pushed the jasnell:broadcastchannel branch Nov 30, 2020
@jasnell jasnell marked this pull request as ready for review Nov 30, 2020
@jasnell
Copy link
Member Author

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

Copy link
Member

@addaleax addaleax left a comment

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 jasnell force-pushed the jasnell:broadcastchannel branch Nov 30, 2020
@jasnell jasnell requested a review from addaleax Nov 30, 2020
@jasnell
Copy link
Member Author

@jasnell jasnell commented Nov 30, 2020

@addaleax .. updated! :-)

Copy link
Member

@addaleax addaleax left a comment

Nice :)

src/node_messaging.cc Outdated Show resolved Hide resolved
@jasnell jasnell force-pushed the jasnell:broadcastchannel branch Nov 30, 2020
Signed-off-by: James M Snell <jasnell@gmail.com>

Closes the `BroadcastChannel` connection.

### `broadcastChannel.onmessage`

This comment has been minimized.

@benjamingr

benjamingr Dec 1, 2020
Member

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

This comment has been minimized.

@jasnell

jasnell Dec 1, 2020
Author Member

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.

This comment has been minimized.

@benjamingr

benjamingr Dec 1, 2020
Member

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

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 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
@addaleax addaleax mentioned this pull request Dec 1, 2020
2 of 2 tasks complete
@Trott
Copy link
Member

@Trott 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 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 added 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 added 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 added 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 added 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants