Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Commit 2ca42c8

Browse filesBrowse files
authored
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. PR-URL: #61895 Backport-PR-URL: #63194 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent 31a863f commit 2ca42c8
Copy full SHA for 2ca42c8

2 files changed

+49-19Lines changed: 49 additions & 19 deletions

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎lib/repl.js‎

Copy file name to clipboardExpand all lines: lib/repl.js
+32-19Lines changed: 32 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -186,7 +186,35 @@ const domainSet = new SafeWeakSet();
186186
const kBufferedCommandSymbol = Symbol('bufferedCommand');
187187
const kLoadingSymbol = Symbol('loading');
188188

189-
let addedNewListener = false;
189+
function processNewListener(event, listener) {
190+
if (event === 'uncaughtException' &&
191+
process.domain &&
192+
listener.name !== 'domainUncaughtExceptionClear' &&
193+
domainSet.has(process.domain)) {
194+
// Throw an error so that the event will not be added and the current
195+
// domain takes over. That way the user is notified about the error
196+
// and the current code evaluation is stopped, just as any other code
197+
// that contains an error.
198+
throw new ERR_INVALID_REPL_INPUT(
199+
'Listeners for `uncaughtException` cannot be used in the REPL');
200+
}
201+
}
202+
203+
let processNewListenerUseCount = 0;
204+
function addProcessNewListener() {
205+
if (processNewListenerUseCount++ === 0) {
206+
// Add this listener only once and use a WeakSet that contains the REPLs
207+
// domains. Otherwise we'd have to add a single listener to each REPL
208+
// instance and that could trigger the `MaxListenersExceededWarning`.
209+
process.prependListener('newListener', processNewListener);
210+
}
211+
}
212+
213+
function removeProcessNewListener() {
214+
if (--processNewListenerUseCount === 0) {
215+
process.removeListener('newListener', processNewListener);
216+
}
217+
}
190218

191219
fixReplRequire(module);
192220

@@ -327,24 +355,9 @@ function REPLServer(prompt,
327355
// It is possible to introspect the running REPL accessing this variable
328356
// from inside the REPL. This is useful for anyone working on the REPL.
329357
module.exports.repl = this;
330-
} else if (!addedNewListener) {
331-
// Add this listener only once and use a WeakSet that contains the REPLs
332-
// domains. Otherwise we'd have to add a single listener to each REPL
333-
// instance and that could trigger the `MaxListenersExceededWarning`.
334-
process.prependListener('newListener', (event, listener) => {
335-
if (event === 'uncaughtException' &&
336-
process.domain &&
337-
listener.name !== 'domainUncaughtExceptionClear' &&
338-
domainSet.has(process.domain)) {
339-
// Throw an error so that the event will not be added and the current
340-
// domain takes over. That way the user is notified about the error
341-
// and the current code evaluation is stopped, just as any other code
342-
// that contains an error.
343-
throw new ERR_INVALID_REPL_INPUT(
344-
'Listeners for `uncaughtException` cannot be used in the REPL');
345-
}
346-
});
347-
addedNewListener = true;
358+
} else {
359+
addProcessNewListener();
360+
this.once('exit', removeProcessNewListener);
348361
}
349362

350363
domainSet.add(this._domain);
Collapse file
+17Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
'use strict';
2+
const common = require('../common');
3+
const { startNewREPLServer } = require('../common/repl');
4+
const assert = require('assert');
5+
6+
const originalProcessNewListenerCount = process.listenerCount('newListener');
7+
const { replServer } = startNewREPLServer();
8+
9+
const listenerCountBeforeClose = process.listenerCount('newListener');
10+
replServer.close();
11+
replServer.once('exit', common.mustCall(() => {
12+
setImmediate(common.mustCall(() => {
13+
const listenerCountAfterClose = process.listenerCount('newListener');
14+
assert.strictEqual(listenerCountAfterClose, listenerCountBeforeClose - 1);
15+
assert.strictEqual(listenerCountAfterClose, originalProcessNewListenerCount);
16+
}));
17+
}));

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.