Skip to content
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

Merged
merged 5 commits into from
Mar 11, 2021

Conversation

nbbeeken
Copy link
Contributor

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

@nbbeeken nbbeeken force-pushed the NODE-3100/fix/sessions-issue branch from b2f0ec4 to 7909c78 Compare February 16, 2021 18:59
@nbbeeken nbbeeken requested review from durran and emadum February 16, 2021 19:29
@nbbeeken nbbeeken marked this pull request as ready for review February 16, 2021 19:29
lib/operations/execute_operation.js Outdated Show resolved Hide resolved
@nbbeeken nbbeeken force-pushed the NODE-3100/fix/sessions-issue branch from 60c0b22 to 904af17 Compare March 3, 2021 16:48
@nbbeeken nbbeeken requested a review from mbroadst March 3, 2021 22:53
@nbbeeken
Copy link
Contributor Author

nbbeeken commented Mar 3, 2021

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.
I've made #2747 to clean up a majority of the CI issues.

nbbeeken added 3 commits March 5, 2021 13:13
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
@nbbeeken nbbeeken force-pushed the NODE-3100/fix/sessions-issue branch from 701530a to 8a0e3ef Compare March 5, 2021 18:13
lib/operations/execute_operation.js Outdated Show resolved Hide resolved
lib/operations/execute_operation.js Outdated Show resolved Hide resolved
lib/operations/execute_operation.js Outdated Show resolved Hide resolved
lib/operations/execute_operation.js Outdated Show resolved Hide resolved
test/unit/sessions/client.test.js Outdated Show resolved Hide resolved
} 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()) {
Copy link
Member

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.

Copy link
Contributor Author

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'
  })
}

Copy link
Member

@mbroadst mbroadst Mar 10, 2021

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?

Copy link
Contributor Author

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!)

Copy link
Contributor

@emadum emadum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀

This was referenced Mar 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants