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>
This commit is contained in:
Anna Henningsen 2019-06-02 15:09:57 +02:00
parent 10a346edde
commit ed24c19002
No known key found for this signature in database
GPG Key ID: 9C63F3A6CD2AD8F9
7 changed files with 50 additions and 23 deletions

View File

@ -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 is deprecated. This deprecation applies to users of the
[`--http-parser=legacy`][] command-line flag. [`--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 workers `'exit'` event.
[`--http-parser=legacy`]: cli.html#cli_http_parser_library [`--http-parser=legacy`]: cli.html#cli_http_parser_library
[`--pending-deprecation`]: cli.html#cli_pending_deprecation [`--pending-deprecation`]: cli.html#cli_pending_deprecation
[`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size [`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.types`]: util.html#util_util_types
[`util`]: util.html [`util`]: util.html
[`worker.exitedAfterDisconnect`]: cluster.html#cluster_worker_exitedafterdisconnect [`worker.exitedAfterDisconnect`]: cluster.html#cluster_worker_exitedafterdisconnect
[`worker.terminate()`]: worker_threads.html#worker_threads_worker_terminate
[`zlib.bytesWritten`]: zlib.html#zlib_zlib_byteswritten [`zlib.bytesWritten`]: zlib.html#zlib_zlib_byteswritten
[Legacy URL API]: url.html#url_legacy_url_api [Legacy URL API]: url.html#url_legacy_url_api
[NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf

View File

@ -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 [`Worker`][] constructor, then data will be piped to the parent thread's
[`process.stdout`][] stream. [`process.stdout`][] stream.
### worker.terminate([callback]) ### worker.terminate()
<!-- YAML <!-- YAML
added: v10.5.0 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} * Returns: {Promise}
* `err` {Error}
* `exitCode` {integer}
Stop all JavaScript execution in the worker thread as soon as possible. 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 Returns a Promise for the exit code that is fulfilled when the
to have completed. [`'exit'` event][] is emitted.
**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.
### worker.threadId ### worker.threadId
<!-- YAML <!-- 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. `unref()` again will have no effect.
[`'close'` event]: #worker_threads_event_close [`'close'` event]: #worker_threads_event_close
[`'exit'` event]: #worker_threads_event_exit
[`AsyncResource`]: async_hooks.html#async_hooks_class_asyncresource [`AsyncResource`]: async_hooks.html#async_hooks_class_asyncresource
[`Buffer`]: buffer.html [`Buffer`]: buffer.html
[`EventEmitter`]: events.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.on('message')`]: #worker_threads_event_message_1
[`worker.postMessage()`]: #worker_threads_worker_postmessage_value_transferlist [`worker.postMessage()`]: #worker_threads_worker_postmessage_value_transferlist
[`worker.SHARE_ENV`]: #worker_threads_worker_share_env [`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 [`worker.threadId`]: #worker_threads_worker_threadid_1
[Addons worker support]: addons.html#addons_worker_support [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 [HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

View File

@ -228,10 +228,22 @@ class Worker extends EventEmitter {
debug(`[${threadId}] terminates Worker with ID ${this.threadId}`); 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.once('exit', (exitCode) => callback(null, exitCode));
}
this[kHandle].stopThread(); 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() { ref() {

View File

@ -352,11 +352,8 @@ void Worker::JoinThread() {
thread_joined_ = true; thread_joined_ = true;
env()->remove_sub_worker_context(this); env()->remove_sub_worker_context(this);
OnThreadStopped();
on_thread_finished_.Uninstall(); on_thread_finished_.Uninstall();
}
void Worker::OnThreadStopped() {
{ {
HandleScope handle_scope(env()->isolate()); HandleScope handle_scope(env()->isolate());
Context::Scope context_scope(env()->context()); Context::Scope context_scope(env()->context());
@ -370,7 +367,7 @@ void Worker::OnThreadStopped() {
MakeCallback(env()->onexit_string(), 1, &code); 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. // the C++ object is no longer needed for anything now.
MakeWeak(); MakeWeak();
} }
@ -534,8 +531,6 @@ void Worker::StopThread(const FunctionCallbackInfo<Value>& args) {
Debug(w, "Worker %llu is getting stopped by parent", w->thread_id_); Debug(w, "Worker %llu is getting stopped by parent", w->thread_id_);
w->Exit(1); w->Exit(1);
w->JoinThread();
delete w;
} }
void Worker::Ref(const FunctionCallbackInfo<Value>& args) { void Worker::Ref(const FunctionCallbackInfo<Value>& args) {

View File

@ -52,7 +52,6 @@ class Worker : public AsyncWrap {
static void Unref(const v8::FunctionCallbackInfo<v8::Value>& args); static void Unref(const v8::FunctionCallbackInfo<v8::Value>& args);
private: private:
void OnThreadStopped();
void CreateEnvMessagePort(Environment* env); void CreateEnvMessagePort(Environment* env);
std::shared_ptr<PerIsolateOptions> per_isolate_opts_; std::shared_ptr<PerIsolateOptions> per_isolate_opts_;

View File

@ -10,5 +10,5 @@ require('worker_threads').parentPort.postMessage('0');
w.on('message', common.mustCall(() => { w.on('message', common.mustCall(() => {
// This should not crash the worker during a DNS request. // This should not crash the worker during a DNS request.
w.terminate(common.mustCall()); w.terminate().then(common.mustCall());
})); }));

View File

@ -12,8 +12,14 @@ process.nextTick(() => {
}); });
`, { eval: true }); `, { 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(() => { w.on('message', common.mustCall(() => {
setTimeout(() => { setTimeout(() => {
w.terminate(common.mustCall()); w.terminate(common.mustCall()).then(common.mustCall());
}, 1); }, 1);
})); }));