worker: refactor `worker.terminate()` · nodejs/node@3611b47 · GitHub
Skip to content

Commit 3611b47

Browse files
addaleaxBridgeAR
addaleax
authored and
BridgeAR
committed
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: 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>
1 parent 83fc10d commit 3611b47

7 files changed

Lines changed: 50 additions & 23 deletions

File tree

‎doc/api/deprecations.md‎

Lines changed: 15 additions & 0 deletions

‎doc/api/worker_threads.md‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -617,24 +617,23 @@ inside the worker thread. If `stdout: true` was not passed to the
617617
[`Worker`][] constructor, then data will be piped to the parent thread's
618618
[`process.stdout`][] stream.
619619

620-
### worker.terminate([callback])
620+
### worker.terminate()
621621
<!-- YAML
622622
added: v10.5.0
623+
changes:
624+
- version: REPLACEME
625+
pr-url: https://github.com/nodejs/node/pull/28021
626+
description: This function now returns a Promise.
627+
Passing a callback is deprecated, and was useless up to this
628+
version, as the Worker was actually terminated synchronously.
629+
Terminating is now a fully asynchronous operation.
623630
-->
624631

625-
* `callback` {Function}
626-
* `err` {Error}
627-
* `exitCode` {integer}
632+
* Returns: {Promise}
628633

629634
Stop all JavaScript execution in the worker thread as soon as possible.
630-
`callback` is an optional function that is invoked once this operation is known
631-
to have completed.
632-
633-
**Warning**: Currently, not all code in the internals of Node.js is prepared to
634-
expect termination at arbitrary points in time and may crash if it encounters
635-
that condition. Consequently, only call `.terminate()` if it is known that the
636-
Worker thread is not accessing Node.js core modules other than what is exposed
637-
in the `worker` module.
635+
Returns a Promise for the exit code that is fulfilled when the
636+
[`'exit'` event][] is emitted.
638637

639638
### worker.threadId
640639
<!-- YAML
@@ -657,6 +656,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
657656
`unref()` again will have no effect.
658657

659658
[`'close'` event]: #worker_threads_event_close
659+
[`'exit'` event]: #worker_threads_event_exit
660660
[`AsyncResource`]: async_hooks.html#async_hooks_class_asyncresource
661661
[`Buffer`]: buffer.html
662662
[`EventEmitter`]: events.html
@@ -690,7 +690,7 @@ active handle in the event system. If the worker is already `unref()`ed calling
690690
[`worker.on('message')`]: #worker_threads_event_message_1
691691
[`worker.postMessage()`]: #worker_threads_worker_postmessage_value_transferlist
692692
[`worker.SHARE_ENV`]: #worker_threads_worker_share_env
693-
[`worker.terminate()`]: #worker_threads_worker_terminate_callback
693+
[`worker.terminate()`]: #worker_threads_worker_terminate
694694
[`worker.threadId`]: #worker_threads_worker_threadid_1
695695
[Addons worker support]: addons.html#addons_worker_support
696696
[HTML structured clone algorithm]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm

‎lib/internal/worker.js‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -228,10 +228,22 @@ class Worker extends EventEmitter {
228228

229229
debug(`[${threadId}] terminates Worker with ID ${this.threadId}`);
230230

231-
if (typeof callback !== 'undefined')
231+
if (typeof callback === 'function') {
232+
process.emitWarning(
233+
'Passing a callback to worker.terminate() is deprecated. ' +
234+
'It returns a Promise instead.',
235+
'DeprecationWarning', 'DEP0XXX');
232236
this.once('exit', (exitCode) => callback(null, exitCode));
237+
}
233238

234239
this[kHandle].stopThread();
240+
241+
// Do not use events.once() here, because the 'exit' event will always be
242+
// emitted regardless of any errors, and the point is to only resolve
243+
// once the thread has actually stopped.
244+
return new Promise((resolve) => {
245+
this.once('exit', resolve);
246+
});
235247
}
236248

237249
ref() {

‎src/node_worker.cc‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -352,11 +352,8 @@ void Worker::JoinThread() {
352352
thread_joined_ = true;
353353

354354
env()->remove_sub_worker_context(this);
355-
OnThreadStopped();
356355
on_thread_finished_.Uninstall();
357-
}
358356

359-
void Worker::OnThreadStopped() {
360357
{
361358
HandleScope handle_scope(env()->isolate());
362359
Context::Scope context_scope(env()->context());
@@ -370,7 +367,7 @@ void Worker::OnThreadStopped() {
370367
MakeCallback(env()->onexit_string(), 1, &code);
371368
}
372369

373-
// JoinThread() cleared all libuv handles bound to this Worker,
370+
// We cleared all libuv handles bound to this Worker above,
374371
// the C++ object is no longer needed for anything now.
375372
MakeWeak();
376373
}
@@ -534,8 +531,6 @@ void Worker::StopThread(const FunctionCallbackInfo<Value>& args) {
534531

535532
Debug(w, "Worker %llu is getting stopped by parent", w->thread_id_);
536533
w->Exit(1);
537-
w->JoinThread();
538-
delete w;
539534
}
540535

541536
void Worker::Ref(const FunctionCallbackInfo<Value>& args) {

‎src/node_worker.h‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,6 @@ class Worker : public AsyncWrap {
5252
static void Unref(const v8::FunctionCallbackInfo<v8::Value>& args);
5353

5454
private:
55-
void OnThreadStopped();
5655
void CreateEnvMessagePort(Environment* env);
5756

5857
std::shared_ptr<PerIsolateOptions> per_isolate_opts_;

‎test/parallel/test-worker-dns-terminate.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,5 +10,5 @@ require('worker_threads').parentPort.postMessage('0');
1010

1111
w.on('message', common.mustCall(() => {
1212
// This should not crash the worker during a DNS request.
13-
w.terminate(common.mustCall());
13+
w.terminate().then(common.mustCall());
1414
}));

‎test/parallel/test-worker-nexttick-terminate.js‎

Lines changed: 7 additions & 1 deletion

0 commit comments

Comments
 (0)