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.

Author: addaleax

lib/repl.js

@@ -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);

test/parallel/test-repl-no-terminal-restore-process-listeners.js

@@ -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);
+  }));
+}));