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.

Sign up for GitHub

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

Jump to bottom

database plugin: Invalidate queue should cancel context first #15933

Merged
merged 1 commit into from Jun 10, 2022
Merged

database plugin: Invalidate queue should cancel context first #15933

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 6 additions & 3 deletions builtin/logical/database/backend.go
View file
Open in desktop
Expand Up @@ -286,12 +286,15 @@ func (b *databaseBackend) GetConnectionWithConfig(ctx context.Context, name stri

// invalidateQueue cancels any background queue loading and destroys the queue.
func (b *databaseBackend) invalidateQueue() {
b.Lock()
defer b.Unlock()

// cancel context before grabbing lock to start closing any open connections
// this is safe to do without the lock since it is only written to once in initialization
// and can be canceled multiple times safely
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
Member

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
Member

@calvn calvn Jun 10, 2022 •
edited

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.

defer b.Unlock()

b.credRotationQueue = nil
}

Expand Down