Skip to content

Commit

Permalink
worker: set trackUnmanagedFds to true by default
Browse files Browse the repository at this point in the history
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 <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
  • Loading branch information
addaleax authored and jasnell committed Jul 22, 2020
1 parent f4f191b commit 7603c7e
Show file tree
Hide file tree
Showing 3 changed files with 19 additions and 3 deletions.
5 changes: 4 additions & 1 deletion doc/api/worker_threads.md
Expand Up @@ -621,6 +621,9 @@ if (isMainThread) {
<!-- YAML
added: v10.5.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/34394
description: The `trackUnmanagedFds` option was set to `true` by default.
- version:
- v14.6.0
pr-url: https://github.com/nodejs/node/pull/34303
Expand Down Expand Up @@ -689,7 +692,7 @@ changes:
[`fs.close()`][], and close them when the Worker exits, similar to other
resources like network sockets or file descriptors managed through
the [`FileHandle`][] API. This option is automatically inherited by all
nested `Worker`s. **Default**: `false`.
nested `Worker`s. **Default**: `true`.
* `transferList` {Object[]} If one or more `MessagePort`-like objects
are passed in `workerData`, a `transferList` is required for those
items or [`ERR_MISSING_MESSAGE_PORT_IN_TRANSFER_LIST`][] will be thrown.
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/worker.js
Expand Up @@ -152,7 +152,7 @@ class Worker extends EventEmitter {
env === process.env ? null : env,
options.execArgv,
parseResourceLimits(options.resourceLimits),
!!options.trackUnmanagedFds);
!!(options.trackUnmanagedFds ?? true));
if (this[kHandle].invalidExecArgv) {
throw new ERR_WORKER_INVALID_EXEC_ARGV(this[kHandle].invalidExecArgv);
}
Expand Down
15 changes: 14 additions & 1 deletion test/parallel/test-worker-track-unmanaged-fds.js
@@ -1,10 +1,13 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const { Worker } = require('worker_threads');
const { Worker, isMainThread } = require('worker_threads');
const { once } = require('events');
const fs = require('fs');

if (!isMainThread)
common.skip('test needs to be able to freely set `trackUnmanagedFds`');

// All the tests here are run sequentially, to avoid accidentally opening an fd
// which another part of the test expects to be closed.

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

// The same, but trackUnmanagedFds is used only as the implied default.
{
const w = new Worker(`${preamble}
parentPort.postMessage(fs.openSync(__filename));
`, { eval: true });
const [ [ fd ] ] = await Promise.all([once(w, 'message'), once(w, 'exit')]);
assert(fd > 2);
assert.throws(() => fs.fstatSync(fd), { code: 'EBADF' });
}

// There is a warning when an fd is unexpectedly opened twice.
{
const w = new Worker(`${preamble}
Expand Down

0 comments on commit 7603c7e

Please sign in to comment.