-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(NODE-6472): findOne and find no longer keep open cursors #4580
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
base: main
Are you sure you want to change the base?
Conversation
11b1cf5
to
9fe1c2d
Compare
src/collection.ts
Outdated
const res = await cursor.next(); | ||
await cursor.close(); | ||
return res; | ||
const opts = { ...options }; |
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 made the decision here to do the options changes - creating a new find one operation was much more code and was suspiciously failing 6 tests in load balancer mode. This creating the cursor object works just fine and passes all tests, no killCursors is sent and the cursor on the server side always comes back with id 0.
@@ -81,6 +81,16 @@ export interface FindOptions<TSchema extends Document = Document> | |||
timeoutMode?: CursorTimeoutMode; | |||
} | |||
|
|||
/** @public */ | |||
export interface FindOneOptions extends FindOptions { | |||
/** @deprecated Will be removed in the next major version. User provided value will be ignored. */ |
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.
Do we have a ticket to remove these fields from FindOneOptions
in v7?
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.
We'd need to add this to the bucket deprecations ticket in V7 or file a new ticket in V7 if the change goes through. (Let's add AC to the ticket to remember to do this). But why are we introducing exposing a brand new interface with deprecated options at all? If we remove these options from the interface, we are going to be back at FindOneOptions being the same as FindOptions - is the intent to replace the definition of this interface in V7 with a copy of FindOptions minus these three?
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 expect that once they're removed, we will have:
/** @public */
export type FindOneOptions = Omit<FindOptions, ....>;
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.
Yeah, I think if we're doing more than just removing deprecations, we should do it in a separate ticket from the bucket work (to streamline the bucketed option removal).
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.
So a separate ticket to NODE-5545? I had added these removals there but can create a another ticket.
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 don't have a very strong preference as far as using the bucket ticket or filing a separate one if the team feels like it's not necessary, but we do want to capture that this won't be strictly deleting the deprecation lines, because I can see us moving quickly down that list in the bucket and inadvertently ending up back at FindOneOptions = FindOptions
@@ -195,7 +205,13 @@ function makeFindCommand(ns: MongoDBNamespace, filter: Document, options: FindOp | |||
|
|||
findCommand.singleBatch = true; | |||
} else { | |||
findCommand.batchSize = options.batchSize; | |||
if (options.batchSize === options.limit) { |
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.
the if
statement above, which set singleBatch: true
was only used for findOne I think. Is that dead code we can remove now?
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.
Good call. I've removed 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.
I also noticed two if-statements above:
if (typeof options.limit === 'number') {
if (options.limit < 0) {
findCommand.limit = -options.limit;
findCommand.singleBatch = true;
} else {
findCommand.limit = options.limit;
}
}
We can remove the first nested if-statement there, right? That seems to have been used for find one with the old findOne implementation:
const cursor = this.find(filter, options).limit(-1).batchSize(1);
delete opts.batchSize; | ||
} | ||
const cursor = this.find(filter, opts).limit(1); | ||
return await cursor.next(); |
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.
Thoughts on closing the cursor here regardless if whether or not it "shouldn't be necessary"? See Bernard's comment: https://jira.mongodb.org/browse/SERVER-80713#:~:text=In%20general%2C%20it%20is%20not%20safe%20or%20advisable%20for%20client%20code%20to%20stop%20iterating%20a%20cursor%20until%20the%20driver%20explicitly%20reports%20that%20the%20cursor%20has%20been%20closed%2C%20even%20if%20the%20client%20app%20%22knows%22%20that%20it%20will%20only%20ever%20see%20a%20single%20result.
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.
Also - one of the AC in the ticket:
Ensure no killCursors command is sent to the server.
Do we test this anywhere? (should we test this?)
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.
The new unified tests added have expectedEvents
that would fail if a killCursors
command was issued, so I considered this acceptable from a testing point. As for the previous point, there is a performance aspect to the ticket, so I felt there was no need to go through the close
logic when in theory it would not be 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.
Close is effectively a no-op when the cursor is already exhausted but if it ever weren't exhausted, we'd be leaking cursors. I think it's worth adding a call to close just in case.
If we're really concerned about throughput, we could even not await close:
cursor.close().then(squashError, squashError);
return result;
Co-authored-by: Bailey Pearson <[email protected]>
Co-authored-by: Bailey Pearson <[email protected]>
Description
Changes
findOne
to set options that will not require a cursor or a killCursors command. Updatesfind
to not leave cursors open on the server when batch size and limit are equal.What is changing?
batchSize
tobatchSize + 1
when the limit is equal to it.Is there new documentation needed for these changes?
API docs on the findOne options.
What is the motivation for this change?
NODE-6472
Release Highlight
collection.findOne()
andcollection.find()
will no longer potentially leave open cursors on the serverThe
findOne
command will now always set thelimit
option to1
andsingleBatch
totrue
. Thelimit
,noCursorResponse
andbatchSize
options have also been deprecated, and the command will guarantee no more cursors can be orphaned and nokillCursors
command will be potentially executed.find
will now setlimit
tobatchSize + 1
when both options were equal, to avoid leaving cursors open.Double check the following
npm run check:lint
scripttype(NODE-xxxx)[!]: description
feat(NODE-1234)!: rewriting everything in coffeescript