Skip to content

Commit

Permalink
worker: refactor worker.terminate()
Browse files Browse the repository at this point in the history
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: openjs-foundation/summit#141

PR-URL: #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>
  • Loading branch information
addaleax authored and targos committed Jun 18, 2019
1 parent 9c19c4b commit d659ed6
Showing 7 changed files with 50 additions and 23 deletions.
15 changes: 15 additions & 0 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
@@ -2477,6 +2477,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
@@ -2569,6 +2583,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
26 changes: 13 additions & 13 deletions doc/api/worker_threads.md
Original file line number Diff line number Diff line change
@@ -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
14 changes: 13 additions & 1 deletion lib/internal/worker.js
Original file line number Diff line number Diff line change
@@ -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() {
7 changes: 1 addition & 6 deletions src/node_worker.cc
Original file line number Diff line number Diff line change
@@ -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) {
1 change: 0 additions & 1 deletion src/node_worker.h
Original file line number Diff line number Diff line change
@@ -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_;
2 changes: 1 addition & 1 deletion test/parallel/test-worker-dns-terminate.js
Original file line number Diff line number Diff line change
@@ -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());
}));
8 changes: 7 additions & 1 deletion test/parallel/test-worker-nexttick-terminate.js
Original file line number Diff line number Diff line change
@@ -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);
}));

0 comments on commit d659ed6

Please sign in to comment.