diff options
author | Anna Henningsen <anna@addaleax.net> | 2019-06-02 15:09:57 +0200 |
---|---|---|
committer | Anna Henningsen <anna@addaleax.net> | 2019-06-17 08:27:17 -0400 |
commit | ed24c19002379063ce100037bfff7ca2da5b6cc8 (patch) | |
tree | ffecdf23eb3adcfe72a626c129fcc715d477fc20 | |
parent | 10a346edde47c872de9afbcb79aa43095064078b (diff) | |
download | android-node-v8-ed24c19002379063ce100037bfff7ca2da5b6cc8.tar.gz android-node-v8-ed24c19002379063ce100037bfff7ca2da5b6cc8.tar.bz2 android-node-v8-ed24c19002379063ce100037bfff7ca2da5b6cc8.zip |
worker: refactor `worker.terminate()`
At the collaborator summit in Berlin, the behaviour of
`worker.terminate()` was discussed.
In particular, switching from a callback-based to a Promise-based API
was suggested. While investigating that possibility later, it was
discovered that `.terminate()` was unintentionally synchronous up
until now (including calling its callback synchronously).
Also, the topic of its stability has been brought up. I have performed
two manual reviews of the native codebase for compatibility with
`.terminate()`, and performed some manual fuzz testing with the test
suite. At this point, bugs with `.terminate()` should, in my opinion,
be treated like bugs in other Node.js features.
(It is possible to make Node.js crash with `.terminate()` by messing
with internals and/or built-in prototype objects, but that is already
the case without `.terminate()` as well.)
This commit:
- Makes `.terminate()` an asynchronous operation.
- Makes `.terminate()` return a `Promise`.
- Runtime-deprecates passing a callback.
- Removes a warning about its stability from the documentation.
- Eliminates an unnecessary extra function from the C++ code.
A possible alternative to returning a `Promise` would be to keep the
method synchronous and just drop the callback. Generally, providing
an asynchronous API does provide us with a bit more flexibility.
Refs: https://github.com/nodejs/summit/issues/141
PR-URL: https://github.com/nodejs/node/pull/28021
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
-rw-r--r-- | doc/api/deprecations.md | 15 | ||||
-rw-r--r-- | doc/api/worker_threads.md | 26 | ||||
-rw-r--r-- | lib/internal/worker.js | 14 | ||||
-rw-r--r-- | src/node_worker.cc | 7 | ||||
-rw-r--r-- | src/node_worker.h | 1 | ||||
-rw-r--r-- | test/parallel/test-worker-dns-terminate.js | 2 | ||||
-rw-r--r-- | test/parallel/test-worker-nexttick-terminate.js | 8 |
7 files changed, 50 insertions, 23 deletions
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 69913686a8..af28278f23 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2484,6 +2484,20 @@ The legacy HTTP parser, used by default in versions of Node.js prior to 12.0.0, is deprecated. This deprecation applies to users of the [`--http-parser=legacy`][] command-line flag. +<a id="DEP0XXX"></a> +### DEP0XXX: worker.terminate() with callback +<!-- YAML +changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/28021 + description: Runtime deprecation. +--> + +Type: Runtime + +Passing a callback to [`worker.terminate()`][] is deprecated. Use the returned +`Promise` instead, or a listener to the worker’s `'exit'` event. + [`--http-parser=legacy`]: cli.html#cli_http_parser_library [`--pending-deprecation`]: cli.html#cli_pending_deprecation [`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size @@ -2576,6 +2590,7 @@ is deprecated. This deprecation applies to users of the [`util.types`]: util.html#util_util_types [`util`]: util.html [`worker.exitedAfterDisconnect`]: cluster.html#cluster_worker_exitedafterdisconnect +[`worker.terminate()`]: worker_threads.html#worker_threads_worker_terminate [`zlib.bytesWritten`]: zlib.html#zlib_zlib_byteswritten [Legacy URL API]: url.html#url_legacy_url_api [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md index 27d4fb2a6a..39d948ccea 100644 --- a/doc/api/worker_threads.md +++ b/doc/api/worker_threads.md @@ -617,24 +617,23 @@ inside the worker thread. If `stdout: true` was not passed to the [`Worker`][] constructor, then data will be piped to the parent thread's [`process.stdout`][] stream. -### worker.terminate([callback]) +### worker.terminate() <!-- YAML added: v10.5.0 +changes: + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/28021 + description: This function now returns a Promise. + Passing a callback is deprecated, and was useless up to this + version, as the Worker was actually terminated synchronously. + Terminating is now a fully asynchronous operation. --> -* `callback` {Function} - * `err` {Error} - * `exitCode` {integer} +* Returns: {Promise} Stop all JavaScript execution in the worker thread as soon as possible. -`callback` is an optional function that is invoked once this operation is known -to have completed. - -**Warning**: Currently, not all code in the internals of Node.js is prepared to -expect termination at arbitrary points in time and may crash if it encounters -that condition. Consequently, only call `.terminate()` if it is known that the -Worker thread is not accessing Node.js core modules other than what is exposed -in the `worker` module. +Returns a Promise for the exit code that is fulfilled when the +[`'exit'` event][] is emitted. ### worker.threadId <!-- YAML @@ -657,6 +656,7 @@ active handle in the event system. If the worker is already `unref()`ed calling `unref()` again will have no effect. [`'close'` event]: #worker_threads_event_close +[`'exit'` event]: #worker_threads_event_exit [`AsyncResource`]: async_hooks.html#async_hooks_class_asyncresource [`Buffer`]: buffer.html [`EventEmitter`]: events.html @@ -690,7 +690,7 @@ active handle in the event system. If the worker is already `unref()`ed calling [`worker.on('message')`]: #worker_threads_event_message_1 [`worker.postMessage()`]: #worker_threads_worker_postmessage_value_transferlist [`worker.SHARE_ENV`]: #worker_threads_worker_share_env -[`worker.terminate()`]: #worker_threads_worker_terminate_callback +[`worker.terminate()`]: #worker_threads_worker_terminate [`worker.threadId`]: #worker_threads_worker_threadid_1 [Addons worker support]: addons.html#addons_worker_support [HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm diff --git a/lib/internal/worker.js b/lib/internal/worker.js index fc2a5eceed..8fbff4fcf2 100644 --- a/lib/internal/worker.js +++ b/lib/internal/worker.js @@ -228,10 +228,22 @@ class Worker extends EventEmitter { debug(`[${threadId}] terminates Worker with ID ${this.threadId}`); - if (typeof callback !== 'undefined') + if (typeof callback === 'function') { + process.emitWarning( + 'Passing a callback to worker.terminate() is deprecated. ' + + 'It returns a Promise instead.', + 'DeprecationWarning', 'DEP0XXX'); this.once('exit', (exitCode) => callback(null, exitCode)); + } this[kHandle].stopThread(); + + // Do not use events.once() here, because the 'exit' event will always be + // emitted regardless of any errors, and the point is to only resolve + // once the thread has actually stopped. + return new Promise((resolve) => { + this.once('exit', resolve); + }); } ref() { diff --git a/src/node_worker.cc b/src/node_worker.cc index c05e3a9c14..8f97f5c351 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -352,11 +352,8 @@ void Worker::JoinThread() { thread_joined_ = true; env()->remove_sub_worker_context(this); - OnThreadStopped(); on_thread_finished_.Uninstall(); -} -void Worker::OnThreadStopped() { { HandleScope handle_scope(env()->isolate()); Context::Scope context_scope(env()->context()); @@ -370,7 +367,7 @@ void Worker::OnThreadStopped() { MakeCallback(env()->onexit_string(), 1, &code); } - // JoinThread() cleared all libuv handles bound to this Worker, + // We cleared all libuv handles bound to this Worker above, // the C++ object is no longer needed for anything now. MakeWeak(); } @@ -534,8 +531,6 @@ void Worker::StopThread(const FunctionCallbackInfo<Value>& args) { Debug(w, "Worker %llu is getting stopped by parent", w->thread_id_); w->Exit(1); - w->JoinThread(); - delete w; } void Worker::Ref(const FunctionCallbackInfo<Value>& args) { diff --git a/src/node_worker.h b/src/node_worker.h index 0e659a9f52..db3c95d2ae 100644 --- a/src/node_worker.h +++ b/src/node_worker.h @@ -52,7 +52,6 @@ class Worker : public AsyncWrap { static void Unref(const v8::FunctionCallbackInfo<v8::Value>& args); private: - void OnThreadStopped(); void CreateEnvMessagePort(Environment* env); std::shared_ptr<PerIsolateOptions> per_isolate_opts_; diff --git a/test/parallel/test-worker-dns-terminate.js b/test/parallel/test-worker-dns-terminate.js index 381c211a88..da9d543c3b 100644 --- a/test/parallel/test-worker-dns-terminate.js +++ b/test/parallel/test-worker-dns-terminate.js @@ -10,5 +10,5 @@ require('worker_threads').parentPort.postMessage('0'); w.on('message', common.mustCall(() => { // This should not crash the worker during a DNS request. - w.terminate(common.mustCall()); + w.terminate().then(common.mustCall()); })); diff --git a/test/parallel/test-worker-nexttick-terminate.js b/test/parallel/test-worker-nexttick-terminate.js index 269509178f..9a22605bb5 100644 --- a/test/parallel/test-worker-nexttick-terminate.js +++ b/test/parallel/test-worker-nexttick-terminate.js @@ -12,8 +12,14 @@ process.nextTick(() => { }); `, { eval: true }); +// Test deprecation of .terminate() with callback. +common.expectWarning( + 'DeprecationWarning', + 'Passing a callback to worker.terminate() is deprecated. ' + + 'It returns a Promise instead.', 'DEP0XXX'); + w.on('message', common.mustCall(() => { setTimeout(() => { - w.terminate(common.mustCall()); + w.terminate(common.mustCall()).then(common.mustCall()); }, 1); })); |