From 9b353c67154d2d0f2786b095b81672eabd4bec32 Mon Sep 17 00:00:00 2001
From: Anna Henningsen <anna@addaleax.net>
Date: Thu, 16 Jul 2020 14:38:21 +0200
Subject: [PATCH] worker: set trackUnmanagedFds to true by default
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This prevents accidental resource leaks when terminating or exiting
Worker that own FDs opened through `fs.open()`.

Refs: https://github.com/nodejs/node/pull/34303

PR-URL: https://github.com/nodejs/node/pull/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>
---
 doc/api/worker_threads.md                        |  5 ++++-
 lib/internal/worker.js                           |  2 +-
 test/parallel/test-worker-track-unmanaged-fds.js | 15 ++++++++++++++-
 3 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/doc/api/worker_threads.md b/doc/api/worker_threads.md
index 7162225c8b1867..db00f7e713c8e0 100644
--- 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
index a0608277158735..7f212f365cb3d4 100644
--- 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
index 88ac8ae751a656..05b54208191520 100644
--- 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}