Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 3611b47

Browse filesBrowse 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
Copy full SHA for 3611b47

File tree

Expand file treeCollapse file tree

7 files changed

+50
-23
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

7 files changed

+50
-23
lines changed
Open diff view settings
Collapse file

‎doc/api/deprecations.md‎

Copy file name to clipboardExpand all lines: doc/api/deprecations.md
+15Lines changed: 15 additions & 0 deletions
  • Display the source diff
  • Display the rich diff
Original file line numberDiff line numberDiff line change
@@ -2477,6 +2477,20 @@ The legacy HTTP parser, used by default in versions of Node.js prior to 12.0.0,
24772477
is deprecated. This deprecation applies to users of the
24782478
[`--http-parser=legacy`][] command-line flag.
24792479
2480+
<a id="DEP0XXX"></a>
2481+
### DEP0XXX: worker.terminate() with callback
2482+
<!-- YAML
2483+
changes:
2484+
- version: REPLACEME
2485+
pr-url: https://github.com/nodejs/node/pull/28021
2486+
description: Runtime deprecation.
2487+
-->
2488+
2489+
Type: Runtime
2490+
2491+
Passing a callback to [`worker.terminate()`][] is deprecated. Use the returned
2492+
`Promise` instead, or a listener to the worker’s `'exit'` event.
2493+
24802494
[`--http-parser=legacy`]: cli.html#cli_http_parser_library
24812495
[`--pending-deprecation`]: cli.html#cli_pending_deprecation
24822496
[`Buffer.allocUnsafeSlow(size)`]: buffer.html#buffer_class_method_buffer_allocunsafeslow_size
@@ -2569,6 +2583,7 @@ is deprecated. This deprecation applies to users of the
25692583
[`util.types`]: util.html#util_util_types
25702584
[`util`]: util.html
25712585
[`worker.exitedAfterDisconnect`]: cluster.html#cluster_worker_exitedafterdisconnect
2586+
[`worker.terminate()`]: worker_threads.html#worker_threads_worker_terminate
25722587
[`zlib.bytesWritten`]: zlib.html#zlib_zlib_byteswritten
25732588
[Legacy URL API]: url.html#url_legacy_url_api
25742589
[NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf
Collapse file

‎doc/api/worker_threads.md‎

Copy file name to clipboardExpand all lines: doc/api/worker_threads.md
+13-13Lines changed: 13 additions & 13 deletions
  • Display the source diff
  • Display the rich diff
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
Collapse file

‎lib/internal/worker.js‎

Copy file name to clipboardExpand all lines: lib/internal/worker.js
+13-1Lines 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() {
Collapse file

‎src/node_worker.cc‎

Copy file name to clipboardExpand all lines: src/node_worker.cc
+1-6Lines 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) {
Collapse file

‎src/node_worker.h‎

Copy file name to clipboardExpand all lines: src/node_worker.h
-1Lines 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_;
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-worker-dns-terminate.js
+1-1Lines 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
}));
Collapse file

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

Copy file name to clipboardExpand all lines: test/parallel/test-worker-nexttick-terminate.js
+7-1Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,8 +12,14 @@ process.nextTick(() => {
1212
});
1313
`, { eval: true });
1414

15+
// Test deprecation of .terminate() with callback.
16+
common.expectWarning(
17+
'DeprecationWarning',
18+
'Passing a callback to worker.terminate() is deprecated. ' +
19+
'It returns a Promise instead.', 'DEP0XXX');
20+
1521
w.on('message', common.mustCall(() => {
1622
setTimeout(() => {
17-
w.terminate(common.mustCall());
23+
w.terminate(common.mustCall()).then(common.mustCall());
1824
}, 1);
1925
}));

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.