Skip to content

Commit 7603c7e

Browse files
addaleaxjasnell
authored andcommitted
worker: set trackUnmanagedFds to true by default
This prevents accidental resource leaks when terminating or exiting Worker that own FDs opened through `fs.open()`. Refs: #34303 PR-URL: #34394 Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>
1 parent f4f191b commit 7603c7e

File tree

3 files changed

+19
-3
lines changed

3 files changed

+19
-3
lines changed

Diff for: doc/api/worker_threads.md

+4-1
Original file line numberDiff line numberDiff line change
@@ -621,6 +621,9 @@ if (isMainThread) {
621621
<!-- YAML
622622
added: v10.5.0
623623
changes:
624+
- version: REPLACEME
625+
pr-url: https://github.com/nodejs/node/pull/34394
626+
description: The `trackUnmanagedFds` option was set to `true` by default.
624627
- version:
625628
- v14.6.0
626629
pr-url: https://github.com/nodejs/node/pull/34303
@@ -689,7 +692,7 @@ changes:
689692
[`fs.close()`][], and close them when the Worker exits, similar to other
690693
resources like network sockets or file descriptors managed through
691694
the [`FileHandle`][] API. This option is automatically inherited by all
692-
nested `Worker`s. **Default**: `false`.
695+
nested `Worker`s. **Default**: `true`.
693696
* `transferList` {Object[]} If one or more `MessagePort`-like objects
694697
are passed in `workerData`, a `transferList` is required for those
695698
items or [`ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST`][] will be thrown.

Diff for: lib/internal/worker.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ class Worker extends EventEmitter {
152152
env === process.env ? null : env,
153153
options.execArgv,
154154
parseResourceLimits(options.resourceLimits),
155-
!!options.trackUnmanagedFds);
155+
!!(options.trackUnmanagedFds ?? true));
156156
if (this[kHandle].invalidExecArgv) {
157157
throw new ERR_WORKER_INVALID_EXEC_ARGV(this[kHandle].invalidExecArgv);
158158
}

Diff for: test/parallel/test-worker-track-unmanaged-fds.js

+14-1
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,13 @@
11
'use strict';
22
const common = require('../common');
33
const assert = require('assert');
4-
const { Worker } = require('worker_threads');
4+
const { Worker, isMainThread } = require('worker_threads');
55
const { once } = require('events');
66
const fs = require('fs');
77

8+
if (!isMainThread)
9+
common.skip('test needs to be able to freely set `trackUnmanagedFds`');
10+
811
// All the tests here are run sequentially, to avoid accidentally opening an fd
912
// which another part of the test expects to be closed.
1013

@@ -37,6 +40,16 @@ process.on('warning', (warning) => parentPort.postMessage({ warning }));
3740
assert.throws(() => fs.fstatSync(fd), { code: 'EBADF' });
3841
}
3942

43+
// The same, but trackUnmanagedFds is used only as the implied default.
44+
{
45+
const w = new Worker(`${preamble}
46+
parentPort.postMessage(fs.openSync(__filename));
47+
`, { eval: true });
48+
const [ [ fd ] ] = await Promise.all([once(w, 'message'), once(w, 'exit')]);
49+
assert(fd > 2);
50+
assert.throws(() => fs.fstatSync(fd), { code: 'EBADF' });
51+
}
52+
4053
// There is a warning when an fd is unexpectedly opened twice.
4154
{
4255
const w = new Worker(`${preamble}

0 commit comments

Comments
 (0)