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 cjihrig committed Jul 22, 2020
1 parent 29309f6 commit 9b353c6
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
@@ -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 9b353c6

Please sign in to comment.