-
Notifications
You must be signed in to change notification settings - Fork 30k
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: initial implementation #20876
Conversation
This comment has been minimized.
This comment has been minimized.
i'm so heckin' happy to see this 🎉 🎉 🎉 🎉 regarding the whole name thing, why not use |
doc/api/process.md
Outdated
@@ -1204,6 +1213,8 @@ console.log(process.getgroups()); // [ 27, 30, 46, 1000 ] | |||
This function is only available on POSIX platforms (i.e. not Windows or | |||
Android). | |||
|
|||
This feature is not available in [`Worker`][] threads. |
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.
maybe we should set up some sort of doc macro for this so that we can keep them all in sync if we decide to change the formatting of it.
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.
Do we know how to do that?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
lib/worker.js
Outdated
|
||
if (!process.binding('config').experimentalWorker) { | ||
// TODO(addaleax): Is this the right way to do this? | ||
// eslint-disable-next-line no-restricted-syntax |
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 would try and gate it in the nativemodule loader instead
@@ -1375,6 +1375,8 @@ def BuildOptions(): | |||
help="Expect test cases to fail", default=False, action="store_true") | |||
result.add_option("--valgrind", help="Run tests through valgrind", | |||
default=False, action="store_true") | |||
result.add_option("--worker", help="Run parallel tests inside a worker context", |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
doc/api/errors.md
Outdated
<a id="ERR_CLOSED_MESSAGE_PORT"></a> | ||
### ERR_CLOSED_MESSAGE_PORT | ||
|
||
Used when there was an attempt to use a `MessagePort` instance in a closed |
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.
Nit: Remove Used when
.
doc/api/errors.md
Outdated
<a id="ERR_MISSING_PLATFORM_FOR_WORKER"></a> | ||
### ERR_MISSING_PLATFORM_FOR_WORKER | ||
|
||
The V8 platform used by this instance of Node does not support creating Workers. |
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.
Nit: Node
-> Node.js
doc/api/errors.md
Outdated
<a id="ERR_WORKER_DOMAIN"></a> | ||
### ERR_WORKER_DOMAIN | ||
|
||
Used when trying to access the `domain` module inside of a worker thread. |
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.
Remove Used when
and reword:
The `domain` module cannot be used inside of a worker thread.
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.
This was one of two outdated error entries anyway. :) It might be cool if our linter could check that entries in this markdown file are also present in lib/internal/errors.h
or src/node_errors.h
…
doc/api/errors.md
Outdated
<a id="ERR_WORKER_NEED_ABSOLUTE_PATH"></a> | ||
### ERR_WORKER_NEED_ABSOLUTE_PATH | ||
|
||
Used when the path for the main script of a worker is not an absolute path. |
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.
Remove Used when
doc/api/errors.md
Outdated
<a id="ERR_WORKER_OUT_OF_MEMORY"></a> | ||
### ERR_WORKER_OUT_OF_MEMORY | ||
|
||
Used when a worker hits its memory limit. |
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.
A worker has exceeded its memory limit.
doc/api/errors.md
Outdated
Used when a worker hits its memory limit. | ||
|
||
<a id="ERR_WORKER_UNAVAILABLE_FEATURE"></a> | ||
### ERR_WORKER_UNAVAILABLE_FEATURE |
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.
Missing description?
doc/api/errors.md
Outdated
<a id="ERR_WORKER_UNSERIALIZABLE_ERROR"></a> | ||
### ERR_WORKER_UNSERIALIZABLE_ERROR | ||
|
||
Used when all attempts at serializing an uncaught exception from a worker fail. |
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.
All attempts to serialize an uncaught exception from a worker failed.
...or perhaps...
All attempts to serialize an uncaught worker exception failed.
doc/api/errors.md
Outdated
<a id="ERR_WORKER_UNSUPPORTED_EXTENSION"></a> | ||
### ERR_WORKER_UNSUPPORTED_EXTENSION | ||
|
||
Used when the pathname used for the main script of a worker has an |
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.
Remove Used when
doc/api/process.md
Outdated
@@ -918,6 +922,8 @@ console.log(process.env.test); | |||
// => 1 | |||
``` | |||
|
|||
*Note*: `process.env` is read-only in [`Worker`][] threads. |
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.
My opinion only but I prefer we leave out *Note*:
in nearly all cases.
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 believe *Note*:
prefixes were eradicated earlier from the docs.
doc/api/process.md
Outdated
@@ -1030,6 +1036,9 @@ If it is necessary to terminate the Node.js process due to an error condition, | |||
throwing an *uncaught* error and allowing the process to terminate accordingly | |||
is safer than calling `process.exit()`. | |||
|
|||
*Note*: in [`Worker`][] threads, this function stops the current thread rather |
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.
My opinion only but I prefer we leave out *Note*:
in nearly all cases.
doc/api/worker.md
Outdated
added: REPLACEME | ||
--> | ||
|
||
Opposite of `unref`, calling `ref` on a previously `unref`d worker will *not* |
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.
Nit: Add parentheses, so unfref()
and ref()
. Also elsewhere in this doc.
Nit: Change comma to period on this line.
Nit: We use `unref`ed
rather than `unref'd`
in existing documentation so use that here and in line 97, 105, and anywhere else for consistency.
doc/api/worker.md
Outdated
|
||
Opposite of `unref`, calling `ref` on a previously `unref`d worker will *not* | ||
let the program exit if it's the only active handle left (the default behavior). | ||
If the worker is `ref`d calling `ref` again will have no effect. |
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.
Add comma before "calling".
doc/api/worker.md
Outdated
* Returns: {undefined} | ||
|
||
Disables further sending of messages on either side of the connection. | ||
This this method can be called once you know that no further communication |
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.
Typo: "This this"
doc/api/worker.md
Outdated
behind this API, see the [serialization API of the `v8` module][v8.serdes]. | ||
|
||
*Note*: Because the object cloning uses the structured clone algorithm, | ||
non-enumberable properties, property accessors, and object prototypes are |
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.
Typo: non-enumberable -> non-enumerable
doc/api/worker.md
Outdated
- [`process.chdir()`][] as well as `process` methods that set group or user ids | ||
are not available. | ||
- [`process.env`][] is a read-only reference to the environment variables. | ||
- [`process.title`][] can not be modified. |
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.
can not -> cannot
doc/api/worker.md
Outdated
child thread. | ||
|
||
To create custom messaging channels (which is encouraged over using the default | ||
global channel because it facilitates seperation of concerns), users can create |
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.
Typo: seperation -> separation
doc/api/worker.md
Outdated
* data {any} Any JavaScript value that will be cloned and made | ||
available as [`require('worker').workerData`][]. The cloning will occur as | ||
described in the [HTML structured clone algorithm][], and an error will be | ||
thrown if the object can not be cloned (e.g. because it contains |
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.
can not -> cannot
doc/api/worker.md
Outdated
--> | ||
|
||
The `'error'` event is emitted if the worker thread throws an uncaught | ||
expection. In that case, the worker will be terminated. |
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.
Typo: expection -> exception
As the author noted, this is work in progress and still in experimental stage ... you can certainly have more threads than much heavier nodejs processes/instances on any given machine... threads give you shared memory with its pros and cons... without threads, nodejs is designed for I/O bound work like fetching something from db... with threads it can do cpu bound work (potentially in parallel if you design it that way) outside the main thread so node can remain responsive to I/O, while processing in background... but even threads are limited by the number of cores and memory available ... |
@addaleax This landed without proper error documentation. |
When can i expect LTS of thread? Eagerly waiting... |
Workers are likely to be experimental for a while. They'll be in 10.x after it goes LTS but still as experimental. |
Thanks 👍 |
Clean up once all references to an `Isolate*` are gone from the `NodePlatform`, rather than waiting for the `PerIsolatePlatformData` struct to be deleted since there may be cyclic references between that struct and the individual tasks. PR-URL: nodejs#20876 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
Currently the following compiler warnings are generated: In file included from ../src/node_platform.cc:1: ../src/node_platform.h:83:16: warning: private field 'isolate_' is not used [-Wunused-private-field] v8::Isolate* isolate_; ^ 1 warning generated. This commit removes these unused private member. PR-URL: nodejs#20876 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Shingo Inoue <leko.noor@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com> Reviewed-By: Gus Caplan <me@gus.host>
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: nodejs#20876
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: nodejs#20876
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: nodejs#20876
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: nodejs#20876
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: #20876 PR-URL: #41616 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: #20876 PR-URL: #41616 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: nodejs#20876 PR-URL: nodejs#41616 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: #20876 PR-URL: #41616 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: #20876 PR-URL: #41616 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: #20876 PR-URL: #41616 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
The current documentation is incorrect in that it says "A single instance of Node.js runs in a single thread," which is not true due to the addition of worker threads. This patch removes the incorrect statement and instead suggests that applications consider using worker threads when process isolation is not needed. Refs: #20876 PR-URL: #41616 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Mestery <mestery@protonmail.com>
Hi everyone! 👋
This PR adds threading support for to Node.js. I realize that this is not exactly a small PR and is going to take a while to review, so: I appreciate comments, questions (any kind, as long as it’s somewhat related 😺), partial reviews and all other feedback from anybody, not just Node.js core collaborators.
The super-high-level description of the implementation here is that Workers can share and transfer memory, but not JS objects (they have to be cloned for transferring), and not yet handles like network sockets.
FAQ
See https://gist.github.com/benjamingr/3d5e86e2fb8ae4abe2ab98ffe4758665
Example usage
Feature set
The communication between threads largely builds on the MessageChannel Web API. Transferring
ArrayBuffer
s and sharing memory throughSharedArrayBuffer
s is supported.Almost the entire Node.js core API is
require()
able orimport
able.Some notable differences:
process.chdir()
don’t exist in worker threads.(Keep in mind that PRs can change significantly based on reviews.)
Comparison with
child_process
andcluster
Workers are conceptually very similar to
child_process
andcluster
.Some of the key differences are:
child_process
IPC, we don’t use JSON, but rather do the same thing thatpostMessage()
does in browsers.process.chdir()
) affect per-process state, loading native addons, etc.Benchmarks
A caveat here is that startup performance for Workers using this model is still relatively slow (I don’t have exact numbers, but there’s definitely overhead).
Regarding semverness:
The only breaking change here is the introduction of a new top-level module. The name is currently
worker
, this is not under a scope as suggested in nodejs/TSC#389. It seems like the most natural name for this by far.I’ve reached out to the owner of the
worker
module on npm, who declined to provide the name for this purpose – the package has 57 downloads/week, so whether we consider this semver-major because of that is probably a judgement call.Alternatively, I’d suggest using
workers
– it’s not quite what we’re used to in core (e.g.child_process
), but the corresponding npm package is essentially just a placeholder.Acknowledgements
People I’d like to thank for their code, comments and reviews for this work in its original form, in no particular order:
… and finally @petkaantonov for a lot of inspiration and the ability to compare with previous work on this topic.
Individual commits
src: cleanup per-isolate state on platform on isolate unregister
Clean up once all references to an
Isolate*
are gone from theNodePlatform
, rather than waiting for thePerIsolatePlatformData
struct to be deleted since there may be cyclic references between
that struct and the individual tasks.
src: fix MallocedBuffer move assignment operator
src: break out of timers loop if
!can_call_into_js()
Otherwise, this turns into an infinite loop.
src: simplify handle closing
Remove one extra closing state and use a smart pointer for
deleting
HandleWrap
s.worker: implement
MessagePort
andMessageChannel
Implement
MessagePort
andMessageChannel
along the lines ofthe DOM classes of the same names.
MessagePort
s initiallysupport transferring only
ArrayBuffer
s.worker: support MessagePort passing in messages
Support passing
MessagePort
instances through otherMessagePort
s,as expected by the
MessagePort
spec.worker: add
SharedArrayBuffer
sharingLogic is added to the
MessagePort
mechanism thatattaches hidden objects to those instances when they are transferred
that track their lifetime and maintain a reference count, to make
sure that memory is freed at the appropriate times.
src: add Env::profiler_idle_notifier_started()
src: move DeleteFnPtr into util.h
This is more generally useful than just in a crypto context.
worker: initial implementation
Implement multi-threading support for most of the API.
test: add test against unsupported worker features
worker: restrict supported extensions
Only allow
.js
and.mjs
extensions to provide future-proofingfor file type detection.
src: enable stdio for workers
Provide
stdin
,stdout
andstderr
options for theWorker
constructor, and make these available to the worker thread
under their usual names.
The default for
stdin
is an empty stream, the default forstdout
andstderr
is redirecting to the parent thread’scorresponding stdio streams.
benchmark: port cluster/echo to worker
worker: improve error (de)serialization
Rather than passing errors using some sort of string representation,
do a best effort for faithful serialization/deserialization of
uncaught exception objects.
test,tools: enable running tests under workers
Enable running tests inside workers by passing
--worker
to
tools/test.py
. A number of tests are marked as skipped,or have been slightly altered to fit the different environment.
Other work
I know that teams from Microsoft (/cc @fs-eire @helloshuangzi) and Alibaba (/cc @aaronleeatali) have been working on forms of multithreading that have higher degrees of interaction between threads, such as sharing code and JS objects. I’d love if you could take a look at this PR and see how well it aligns with your own work, and what conflicts there might be. (From what I’ve seen of the other code, I’m actually quite optimistic that this PR is just going to help everybody.)
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes