-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
feat(NODE-6633): MongoClient.close closes active cursors #4372
base: main
Are you sure you want to change the base?
Conversation
src/cursor/abstract_cursor.ts
Outdated
this.cursorClient.s.activeCursors.add(this); | ||
if (!this.listeners('close').includes(removeActiveCursor)) { | ||
this.once('close', removeActiveCursor); | ||
} |
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.
Any chance that this and the constructor should share more code? This change here feels like something that could have been really easy to miss if you didn't happen to think of it :)
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 "setupTracking" helper of sorts seems worth having, will do, ty
cb798ec
to
567fb98
Compare
3cd7f3f
to
1da872f
Compare
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 pretty good, I have one discussion comment about the explicit clear()
call.
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.
Just a final comment on docs, then should be good to go!
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.
One last nit
Description
What is changing?
Is there new documentation needed for these changes?
What is the motivation for this change?
In an effort to make MongoClient.close more effective and align with other existing close semantics in the ecosystem our close method is being improved to ensure the client when closed cleans up any resources it was responsible for creating. This allows users to rely on the paradigm that a closed client is no longer performing operations, which is currently not true and is likely surprising.
Release Highlight
MongoClient.close now closes any outstanding cursors
Previously, cursors could somewhat live beyond the client they came from. What this meant was depending on timing you would learn of the client's (and by proxy, the cursor's) demise via an assertion that the associated session had expired. This only occurred if your cursor needed to use the session, which only happens when it is attempting to run a
getMore
operation to obtain another batch of documents.Practically speaking a cursor that lives beyond a client is an exception waiting to happen, the connection pools are closed, the sessions are ended, last call has been served 🍻, it is only a matter of timing and event firing until the cursor learns of its fate and informs you via whatever interaction is being used (
.toArray()
,for-await
,.next()
).To make the expected state of cursors clearer in this scenario
MongoClient's
will now close any associated cursor upon itsclose()
-ing reducing the risk of leaving behind server-side resources.Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript