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(NODE-5588): recursive calls to next cause memory leak #3841
Conversation
a2eccf7
to
d87cc39
Compare
d87cc39
to
7d45ad4
Compare
@@ -220,6 +220,11 @@ export abstract class AbstractCursor< | |||
return this[kId] ?? undefined; | |||
} | |||
|
|||
/** @internal */ | |||
get isDead() { |
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.
This makes much more sense to me to be a property of the cursor itself instead of passing a cursor to a function.
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.
Did we intentionally change the semantics of isDead
?
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.
Nevermind, I see that now in the description. But I'm still unsure why the logic was changed
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.
I changed it to exit the loop and enter clean-up logic at the correct times. The prior logic didn't account for a cursor already being closed, or killed, so the loop would attempt a getMore on a killed cursor id. This same "exit" condition applies to all the places the cursorIsDead helper was being used.
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.
Heap looks good, using bind
cleans it up nicely. Well done.
Thanks! I'd like us to pull back in #3804 after this is in to make them actually async functions |
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.
two non-blocking questions, otherwise LGTM.
@@ -101,7 +101,9 @@ export class FindCursor<TSchema = any> extends AbstractCursor<TSchema> { | |||
limit && limit > 0 && numReturned + batchSize > limit ? limit - numReturned : batchSize; | |||
|
|||
if (batchSize <= 0) { | |||
this.close().finally(() => callback()); | |||
this.close().finally(() => |
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.
Is this change just for consistency in return from _getMore
or is there a reason it's needed?
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.
Yea, it is for result consistency, I may be able to revert it if I make sure the logic is nullish safe in next but the contract should have been either throw or result and when we make _getMore actually async that will be the case for the promise's resolution type.
This code path also specifically claims it is for legacy servers which we don't support, I'm fairly certain this is only triggered by mock server tests.
@@ -220,6 +220,11 @@ export abstract class AbstractCursor< | |||
return this[kId] ?? undefined; | |||
} | |||
|
|||
/** @internal */ | |||
get isDead() { |
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.
Did we intentionally change the semantics of isDead
?
Description
What is changing?
(this[kId]?.isZero() ?? false) || this[kClosed] || this[kKilled];
Is there new documentation needed for these changes?
No
Release Highlight
Fix memory leak with ChangeStreams
In a previous release, 5.7.0, we refactored cursor internals from callbacks to async/await. In particular, the
next
function that powers cursors was written with callbacks and would recursively call itself depending on the cursor type. ForChangeStreams
, this function would call itself if there were no new changes to return to the user. After converting that code to async/await each recursive call created a new promise that saved the current async context. This would slowly build up memory usage if no new changes came in to unwind the recursive calls.The function is now implemented as a loop, memory leak be gone!
Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript