Repository: nodejs/node
Pull request: repl: keep reference count for `process.on('newListener')`

When investigating a memory leak in one of our applications, we discovered that this listener holds on to a `REPLServer` instance and all heap objects transitively kept alive by it by capturing as part of its closure.

It's cleaner to declare the listener outside of the `REPLServer` class and to actually clean it up properly when it is no longer required or meaningful, which is easily achieved through keeping a reference count.

<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: lib/repl.js
patch: @@ -183,7 +183,35 @@ const domainSet = new SafeWeakSet();
 const kBufferedCommandSymbol = Symbol('bufferedCommand');
 const kLoadingSymbol = Symbol('loading');
 
-let addedNewListener = false;
+function processNewListener(event, listener) {
+  if (event === 'uncaughtException' &&
+      process.domain &&
+      listener.name !== 'domainUncaughtExceptionClear' &&
+      domainSet.has(process.domain)) {
+    // Throw an error so that the event will not be added and the current
+    // domain takes over. That way the user is notified about the error
+    // and the current code evaluation is stopped, just as any other code
+    // that contains an error.
+    throw new ERR_INVALID_REPL_INPUT(
+      'Listeners for `uncaughtException` cannot be used in the REPL');
+  }
+}
+
+let processNewListenerUseCount = 0;
+function addProcessNewListener() {
+  if (processNewListenerUseCount++ === 0) {
+    // Add this listener only once and use a WeakSet that contains the REPLs
+    // domains. Otherwise we'd have to add a single listener to each REPL
+    // instance and that could trigger the `MaxListenersExceededWarning`.
+    process.prependListener('newListener', processNewListener);
+  }
+}
+
+function removeProcessNewListener() {
+  if (--processNewListenerUseCount === 0) {
+    process.removeListener('newListener', processNewListener);
+  }
+}
 
 fixReplRequire(module);
 
@@ -337,24 +365,9 @@ class REPLServer extends Interface {
       // It is possible to introspect the running REPL accessing this variable
       // from inside the REPL. This is useful for anyone working on the REPL.
       module.exports.repl = this;
-    } else if (!addedNewListener) {
-      // Add this listener only once and use a WeakSet that contains the REPLs
-      // domains. Otherwise we'd have to add a single listener to each REPL
-      // instance and that could trigger the `MaxListenersExceededWarning`.
-      process.prependListener('newListener', (event, listener) => {
-        if (event === 'uncaughtException' &&
-          process.domain &&
-          listener.name !== 'domainUncaughtExceptionClear' &&
-          domainSet.has(process.domain)) {
-          // Throw an error so that the event will not be added and the current
-          // domain takes over. That way the user is notified about the error
-          // and the current code evaluation is stopped, just as any other code
-          // that contains an error.
-          throw new ERR_INVALID_REPL_INPUT(
-            'Listeners for `uncaughtException` cannot be used in the REPL');
-        }
-      });
-      addedNewListener = true;
+    } else {
+      addProcessNewListener();
+      this.once('exit', removeProcessNewListener);
     }
 
     domainSet.add(this._domain);

filename: test/parallel/test-repl-no-terminal-restore-process-listeners.js
patch: @@ -0,0 +1,17 @@
+'use strict';
+const common = require('../common');
+const { startNewREPLServer } = require('../common/repl');
+const assert = require('assert');
+
+const originalProcessNewListenerCount = process.listenerCount('newListener');
+const { replServer } = startNewREPLServer();
+
+const listenerCountBeforeClose = process.listenerCount('newListener');
+replServer.close();
+replServer.once('exit', common.mustCall(() => {
+  setImmediate(common.mustCall(() => {
+    const listenerCountAfterClose = process.listenerCount('newListener');
+    assert.strictEqual(listenerCountAfterClose, listenerCountBeforeClose - 1);
+    assert.strictEqual(listenerCountAfterClose, originalProcessNewListenerCount);
+  }));
+}));