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

database plugin: Invalidate queue should cancel context first #15933

Merged
merged 1 commit into from
Jun 10, 2022

Conversation

swenson
Copy link
Contributor

@swenson swenson commented Jun 10, 2022

To signal to any credentials rotating goroutines that they should cancel
pending operations, which reduces lock contention.

To signal to any credentials rotating goroutines that they should cancel
pending operations, which reduces lock contention.
@swenson swenson requested a review from a team June 10, 2022 19:23
if b.cancelQueue != nil {
b.cancelQueue()
}
b.Lock()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Technically I would argue we don't need the lock, and we don't need to nil out credRotationQueue, since that field is only initialized by Factory, so it's not like databaseBackend can get usefully recycled. But it's probably safest to make the minimal change, so ignore me.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1, doesn't seem like the lock is needed. I'm also okay with leaving it.

Copy link
Contributor

@calvn calvn Jun 10, 2022

Choose a reason for hiding this comment

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

That's true, though there are several places where we check whether credRotationQueue nil or not and only perform things like rotation (async) if non-nil so not sure if it'd impact those calls if we were to skip setting it to nil 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.

Like @calvn says, I think we have to leave the lock since there are several times when we check if credRotationQueue is nil and if not, then read from it. Not having the lock would introduce a potential (unlikely) race condition where we set credRotationQueue nil here, but after the != nil check elsewhere but before they use credRotationQueue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The more I think about it, the more I think we can safely use a different pattern instead of doing the credRotationQueue != nil pattern. Something like:

select {
case <-b.ctx.Done():
default:
  // credRotationQueue is valid, use it
}

And instead of setting credRotationQueue = nil, we just rely on the context that we're canceling anyway.

I'll see if I can take a stab at this, and limiting the use of the other lock, in another PR I think.

@swenson
Copy link
Contributor Author

swenson commented Jun 10, 2022

Thanks everyone!

@swenson swenson merged commit 28119df into main Jun 10, 2022
@swenson swenson deleted the vault-6497/invalidate-queue-cancel branch June 10, 2022 20:41
swenson pushed a commit that referenced this pull request Jun 10, 2022
Following up from discussions in #15923 and #15933, I wanted to split
out a separate PR that drastically reduced the complexity of the use of
the databaseBackend lock. We no longer need it at all for the
`credRotationQueue`, and we can move it to be solely used in a few,
small connections map management functions.
swenson added a commit that referenced this pull request Jun 17, 2022
Cleanup and simplify lock usage in database plugin

Following up from discussions in #15923 and #15933, I wanted to split
out a separate PR that drastically reduced the complexity of the use of
the databaseBackend lock. We no longer need it at all for the
`credRotationQueue`, and we can move it to be solely used in a few,
small connections map management functions.

Co-authored-by: Calvin Leung Huang <[email protected]>
swenson pushed a commit that referenced this pull request Jul 22, 2022
swenson added a commit that referenced this pull request Jul 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants