-
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
fix: move session support check to operation layer #2739
Conversation
b2f0ec4
to
7909c78
Compare
60c0b22
to
904af17
Compare
Despite what CI looks like at the time of commenting I don't believe any of those failures are related to this fix proposed here. |
Monitor checks run on a timer and network errors cause a reset of the topology that would be corrected in the next cycle of the monitor, we were checking a property that gets updated by this async work. By moving the check to the operations layer we will allow users to obtain a session regardless of support and then emit an error if there is not support when the session is used in an operation. NODE-3100
701530a
to
8a0e3ef
Compare
} else if (operation.session.hasEnded) { | ||
throw new MongoError('Use of expired sessions is not permitted'); | ||
return maybePromise(topology, cb, callback => { | ||
if (isUnifiedTopology(topology) && topology.shouldCheckForSessionSupport()) { |
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.
sorry to be so nitpicky, but I prefer what we do in the 4.0 branch where there's a maybePromise
for each of these cases. The way this is written right now, you might end up nesting promises, whereas in 4.0 if you haven't provided a callback we are using the callback from the top maybePromise
to pass into the secondary executeOperation
.
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.
(No worries, please nitpick away!)
I don't see how you can nest promises here since selectServerForSessionSupport
will get passed a defined callback from maybePromise
and so the next call to executeOperation will have that callback defined so the "second" maybePromise
won't create a new Promise.
Something to consider is that this provides one point of exit to executeOperation. Entirely an 🧅 but that pattern is preferable to what is on 4.0. It's a strange pattern to me to have more code run "synchronously" in the remainder of the function after you've returned a function that appears to do an async task. (In this instance, I know maybePromise is all sync it's just wrapping a bit of setup work, but my style point remains). I'm glossing over the callback detail by just returning but we get the idea.
function runOp() {
if (cond)
return doThing(() => {
// now we've done something
return 'one way'
})
// do more things
return doThing(() => {
// now we've done something
return 'the other way'
})
}
vs.
function runOp() {
return doThing(() => {
if (cond) return 'one way'
// do more things
return 'the other way'
})
}
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.
Hm, you're right there's no Promise nesting. Maybe my ultimate concern is just about the multiple maybePromise
calls in the executeOperation
stacktrace. A call to insertOne
right now will look like:
- executeOperation
- maybePromise
- selectServerForSessionSupport
- executeOperation
- maybePromise
- ....
I guess we could provide a further abstraction of executeOperation
(executeOperationImpl
or something) where you could skip one of the maybePromise
, but maybe it's not so much of a concern here. It's unlikely that selectServerForSessionSupport
is called many times, so this isn't a big problem.
Ok! Sounds good to me, maybe you want to make a similar change to the 4.0 branch while you're here?
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 we'll need to make a port of this sessions related fix to 4.0 so I can do that when I'm there (If anyone else reviewing has a differing opinion please feel free to chime in!)
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.
LGTM 🚀
Monitor checks run on a timer and network errors cause a reset
of the topology that would be corrected in the next cycle of the
monitor, we were checking a property that gets updated by this async
work. By moving the check to the operations layer we will allow users
to obtain a session regardless of support and then emit an error if
there is not support when the session is used in an operation.
NODE-3100