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]>
nodejs · Jul 22, 2020 · 7603c7e · 7603c7e
1 parent f4f191b
commit 7603c7e
Show file tree

Hide file tree

Showing 3 changed files with 19 additions and 3 deletions.
diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
@@ -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
@@ -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.

diff --git a/lib/internal/worker.js b/lib/internal/worker.js
@@ -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);
     }

diff --git a/test/parallel/test-worker-track-unmanaged-fds.js b/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.
 
@@ -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}