-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix memory leak in ReliableTopicListenerRunner.next
API-1514
#1377
Fix memory leak in ReliableTopicListenerRunner.next
API-1514
#1377
Conversation
ReliableTopicListenerRunner.next
API-1564ReliableTopicListenerRunner.next
API-1514
Codecov Report
@@ Coverage Diff @@
## master #1377 +/- ##
==========================================
+ Coverage 93.25% 93.27% +0.01%
==========================================
Files 464 464
Lines 16375 16375
Branches 1330 1330
==========================================
+ Hits 15271 15274 +3
+ Misses 805 803 -2
+ Partials 299 298 -1
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
ReliableTopicListenerRunner.next
API-1514ReliableTopicListenerRunner.next
API-1514
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@srknzl I think it totally makes sense.
consider the following example.
function next() {
readMany()
.catch(() => {
//next();
//process.nextTick(() => next());
//setImmediate(() => next());
});
}
function readMany() {
return Promise.reject('client offline');
}
next();
setInterval(() => console.log('interval'), 10);
Now if you use anything other than the setImmediate in the catch block, you wouldn't see the interval printed out.
I believe your reasoning is correct.
- If you use
next
, it becomes something of a recursive function that never ends. Created invocation promise is immediately rejected since the client is offline and reconnect mode is async, and we immediately execute the catch block. - same happens if you use the process.nextTick. That will effectively call
next
once this function ends.
These two options basically block the event loop, and can cause various problems.
setImmediate
allows us to not block the event loop, and still call next infinitely many times.
Thanks for the quick review guys! |
I will send backports of this if the issue happens in older clients as well. |
The leak was due to nested promises triggering each other. In the leak scenario, the catch block of
this.ringbuffer.readMany
call innext()
callsnext
immediately because it gets aClientOfflineError
due toReconnectMode.ASYNC
. Then infinite number ofnext()
calls happen and the error object and the promises leak memory.See:
See screenshots that are taken using Chrome Dev Tools memory snapshot feature:
The infinitely calling
next()
behaviour is the same with Java. However, looks like in Node.js, creating a promise in a promise's catch() block makes the promise non-garbage collectible somehow.I have tried to use
process.nextTick(this.next.bind(this))
instead of callingthis.next()
as well but it led to the same leak of callingthis.next()
:I am not exactly sure why this is the case but it may be due to the execution order of things in the event loop:
Source: https://nodejs.dev/en/learn/understanding-setimmediate/
Using
setImmediate(this.next.bind(this))
fixed the issue:I realized that garbage collection started to work again.
Script used to test the leak:
Fixes #1347