fix(NODE-5588): recursive calls to next cause memory leak #3841
Conversation
nbbeeken
force-pushed
the
NODE-5502-next-loop
branch
from
a2eccf7
to
d87cc39
Compare
nbbeeken
changed the title
nbbeeken
force-pushed
the
NODE-5502-next-loop
branch
from
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.
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.
Did we intentionally change the semantics of isDead?
There was a problem hiding this comment.
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.
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.
durran
added
the
Primary Review
durran
added
Team Review
Thanks! I'd like us to pull back in #3804 after this is in to make them actually async functions |
| @@ -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.
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.
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.
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
nextfunction 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:lintscripttype(NODE-xxxx)[!]: descriptionfeat(NODE-1234)!: rewriting everything in coffeescript