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

worker: support MessagePort to workers data #32278

Closed

Conversation

juanarbol
Copy link
Member

@juanarbol juanarbol commented Mar 15, 2020

This adds support to send MessagePort-like objects within the Workers workerData, the worker itself will be initialized with the passed MessagePort instead of sending port.postMessage(..., messagePort) after initializing the worker.

Fixes: #32250

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 the worker Issues and PRs related to Worker support. label Mar 15, 2020
@@ -185,6 +185,8 @@ class Worker extends EventEmitter {
this[kParentSideStdio] = { stdin, stdout, stderr };

const { port1, port2 } = new MessageChannel();
const transferList = [port2];
if (options.transferList) transferList.push(...options.transferList);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if (options.transferList) transferList.push(...options.transferList);
if (ArrayIsArray(options.transferList)) transferList.push(...options.transferList);

Copy link
Member Author

Choose a reason for hiding this comment

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

Improved! Thank you!!!

Copy link
Member

Choose a reason for hiding this comment

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

@juanarbol @himself65 Hate to be late to this, but can we please undo this? I don’t see any reason why we wouldn’t accept any iterable here. We’re also quite flexible for the transferList argument for .postMessage() in accordance with the web spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem!

const transferList = [channel1.port1, channel2.port1];
new Worker(`${meowScript}`, { eval: true, workerData, transferList });
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
{
const uint8Array = new Uint8Array([ 1, 2, 3, 4 ]);
assert.deepStrictEqual(uint8Array.length, 4);
new Worker(`
const { parentPort, workerData } = require('worker_threads');
parentPort.postMessage(workerData);
`, {
eval: true,
workerData: uint8Array,
transferList: [uint8Array.buffer]
}).on(
'message',
(message) =>
assert.deepStrictEqual(message, Uint8Array.of(1, 2, 3, 4))
);
assert.deepStrictEqual(uint8Array.length, 0);
}

Copy link
Member

Choose a reason for hiding this comment

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

semicolon, excuse me.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, thanks for the the new test and semicolon is not required (make lint-js passed on my machine)

doc/api/worker_threads.md Outdated Show resolved Hide resolved
lib/internal/worker.js Outdated Show resolved Hide resolved
@juanarbol
Copy link
Member Author

ping @jasnell

@jasnell
Copy link
Member

jasnell commented Mar 24, 2020

Looks good once @himself65 is also happy with it

@himself65
Copy link
Member

@jasnell Yes, LGTM

@juanarbol
Copy link
Member Author

@addaleax I've removed the array validation, with that change, the test for "INVALID_ARG_TYPE" is gone now, as long as we don't care now about if it is specifically an array or not. By the way, thanks for the review!

@addaleax addaleax added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. semver-minor PRs that contain new features and should be released in the next minor version. labels Apr 1, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Apr 1, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/30328/ (:heavy_check_mark:)

@juanarbol
Copy link
Member Author

By the way, I could work on backporting if needed.

@addaleax addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 2, 2020
@addaleax
Copy link
Member

addaleax commented Apr 2, 2020

@juanarbol Two things…

  1. Could you squash the commits and change the commit message here so that it starts with worker:, e.g. worker: support MessagePort in workerData? I’d have done that while landing, but I noticed that another thing needs to be changed too:
  2. Can you add a changes: entry to the documentation for new Worker()? That should usually be done when adding new options.

Thank you! 👍

@juanarbol juanarbol force-pushed the juanarbol/transfer-list-workers branch from b06950a to 7153abf Compare April 2, 2020 17:02
@juanarbol juanarbol force-pushed the juanarbol/transfer-list-workers branch from 7153abf to 787ec67 Compare April 2, 2020 17:06
@juanarbol
Copy link
Member Author

@addaleax Done! Thanks for the review, I've more about introducing new features to Node.

@nodejs-github-bot
Copy link
Collaborator

addaleax pushed a commit that referenced this pull request Apr 2, 2020
PR-URL: #32278
Fixes: #32250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax
Copy link
Member

addaleax commented Apr 2, 2020

Landed in ff2f47d 🙂

@addaleax addaleax closed this Apr 2, 2020
BethGriggs pushed a commit that referenced this pull request Apr 7, 2020
PR-URL: #32278
Fixes: #32250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this pull request Apr 12, 2020
PR-URL: #32278
Fixes: #32250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos added a commit that referenced this pull request Apr 14, 2020
Notable changes:

New file system APIs:
* Added a new function, `fs.readv` (with sync and promisified versions).
  This function takes an array of `ArrayBufferView` elements and will
  write the data it reads sequentially to the buffers
  (Sk Sajidul Kadir). #32356
* A new overload is available for `fs.readSync`, which allows to
  optionally pass any of the `offset`, `length` and `position`
  parameters. #32460

Other changes:
* dns:
  * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with
    `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4
    mapped IPv6 addresses (murgatroid99).
    #32183
* http:
  * The default maximum HTTP header size was changed from 8KB to 16KB
    (rosaxny). #32520
* n-api:
  * Calls to `napi_call_threadsafe_function` from the main thread can
    now return the `napi_would_deadlock` status in certain
    circumstances (Gabriel Schulhof).
    #32689
* util:
  * Added a new `maxStrLength` option to `util.inspect`, to control the
    maximum length of printed strings. Its default value is `Infinity`
    (rosaxny). #32392
* worker:
  * Added support for passing a `transferList` along with `workerData`
    to the `Worker` constructor (Juan José Arboleda).
    #32278

New core collaborators:
With this release, we welcome three new Node.js core collaborators:
* himself65. #32734
* flarna (Gerhard Stoebich). #32620
* mildsunrise (Alba Mendez). #32525

PR-URL: #32813
targos added a commit that referenced this pull request Apr 14, 2020
Notable changes:

New file system APIs:
* Added a new function, `fs.readv` (with sync and promisified versions).
  This function takes an array of `ArrayBufferView` elements and will
  write the data it reads sequentially to the buffers
  (Sk Sajidul Kadir). #32356
* A new overload is available for `fs.readSync`, which allows to
  optionally pass any of the `offset`, `length` and `position`
  parameters. #32460

Other changes:
* dns:
  * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with
    `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4
    mapped IPv6 addresses (murgatroid99).
    #32183
* http:
  * The default maximum HTTP header size was changed from 8KB to 16KB
    (rosaxny). #32520
* n-api:
  * Calls to `napi_call_threadsafe_function` from the main thread can
    now return the `napi_would_deadlock` status in certain
    circumstances (Gabriel Schulhof).
    #32689
* util:
  * Added a new `maxStrLength` option to `util.inspect`, to control the
    maximum length of printed strings. Its default value is `Infinity`
    (rosaxny). #32392
* worker:
  * Added support for passing a `transferList` along with `workerData`
    to the `Worker` constructor (Juan José Arboleda).
    #32278

New core collaborators:
With this release, we welcome three new Node.js core collaborators:
* himself65. #32734
* flarna (Gerhard Stoebich). #32620
* mildsunrise (Alba Mendez). #32525

PR-URL: #32813
targos added a commit that referenced this pull request Apr 14, 2020
Notable changes:

New file system APIs:
* Added a new function, `fs.readv` (with sync and promisified versions).
  This function takes an array of `ArrayBufferView` elements and will
  write the data it reads sequentially to the buffers
  (Sk Sajidul Kadir). #32356
* A new overload is available for `fs.readSync`, which allows to
  optionally pass any of the `offset`, `length` and `position`
  parameters. #32460

Other changes:
* dns:
  * Added the `dns.ALL` flag, that can be passed to `dns.lookup()` with
    `dns.V4MAPPED` to return resolved IPv6 addresses as well as IPv4
    mapped IPv6 addresses (murgatroid99).
    #32183
* http:
  * The default maximum HTTP header size was changed from 8KB to 16KB
    (rosaxny). #32520
* n-api:
  * Calls to `napi_call_threadsafe_function` from the main thread can
    now return the `napi_would_deadlock` status in certain
    circumstances (Gabriel Schulhof).
    #32689
* util:
  * Added a new `maxStrLength` option to `util.inspect`, to control the
    maximum length of printed strings. Its default value is `Infinity`
    (rosaxny). #32392
* worker:
  * Added support for passing a `transferList` along with `workerData`
    to the `Worker` constructor (Juan José Arboleda).
    #32278

New core collaborators:
With this release, we welcome three new Node.js core collaborators:
* himself65. #32734
* flarna (Gerhard Stoebich). #32620
* mildsunrise (Alba Mendez). #32525

PR-URL: #32813
@SimonSchick
Copy link
Contributor

Hey, it seems the docs weren't really updated?

@addaleax
Copy link
Member

Right … sorry, that seems to have been missed. @juanarbol I assume you can open a PR that updates the docs?

@juanarbol
Copy link
Member Author

Sure I can!

juanarbol added a commit to juanarbol/node that referenced this pull request Apr 16, 2020
targos pushed a commit to targos/node that referenced this pull request Apr 25, 2020
PR-URL: nodejs#32278
Fixes: nodejs#32250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Apr 27, 2020
Ref: #32278

PR-URL: #32881
Refs: #32278
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Apr 27, 2020
Ref: #32278

PR-URL: #32881
Refs: #32278
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
Ref: #32278

PR-URL: #32881
Refs: #32278
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BridgeAR pushed a commit that referenced this pull request Apr 28, 2020
Ref: #32278

PR-URL: #32881
Refs: #32278
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Apr 28, 2020
PR-URL: #32278
Fixes: #32250
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
Ref: #32278

PR-URL: #32881
Refs: #32278
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
BethGriggs pushed a commit that referenced this pull request Apr 28, 2020
Ref: #32278

PR-URL: #32881
Refs: #32278
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Apr 30, 2020
Ref: #32278

PR-URL: #32881
Refs: #32278
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@juanarbol juanarbol changed the title lib: support MessagePort to workers data worker: support MessagePort to workers data May 7, 2020
targos pushed a commit that referenced this pull request May 13, 2020
Ref: #32278

PR-URL: #32881
Refs: #32278
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@juanarbol juanarbol deleted the juanarbol/transfer-list-workers branch January 19, 2021 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Worker Threads: Add transferList to Worker constructor options
6 participants