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 999bf22

Browse filesBrowse files
addaleaxaduh95
authored andcommitted
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 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
1 parent f3d3968 commit 999bf22
Copy full SHA for 999bf22

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
@@ -183,7 +183,35 @@ const domainSet = new SafeWeakSet();
183183
const kBufferedCommandSymbol = Symbol('bufferedCommand');
184184
const kLoadingSymbol = Symbol('loading');
185185

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

188216
fixReplRequire(module);
189217

@@ -337,24 +365,9 @@ class REPLServer extends Interface {
337365
// It is possible to introspect the running REPL accessing this variable
338366
// from inside the REPL. This is useful for anyone working on the REPL.
339367
module.exports.repl = this;
340-
} else if (!addedNewListener) {
341-
// Add this listener only once and use a WeakSet that contains the REPLs
342-
// domains. Otherwise we'd have to add a single listener to each REPL
343-
// instance and that could trigger the `MaxListenersExceededWarning`.
344-
process.prependListener('newListener', (event, listener) => {
345-
if (event === 'uncaughtException' &&
346-
process.domain &&
347-
listener.name !== 'domainUncaughtExceptionClear' &&
348-
domainSet.has(process.domain)) {
349-
// Throw an error so that the event will not be added and the current
350-
// domain takes over. That way the user is notified about the error
351-
// and the current code evaluation is stopped, just as any other code
352-
// that contains an error.
353-
throw new ERR_INVALID_REPL_INPUT(
354-
'Listeners for `uncaughtException` cannot be used in the REPL');
355-
}
356-
});
357-
addedNewListener = true;
368+
} else {
369+
addProcessNewListener();
370+
this.once('exit', removeProcessNewListener);
358371
}
359372

360373
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.