Don't use activeContext for standby/leader cancelation #4919
Conversation
vault/ha.go
Outdated
| c.logger.Error("clearing leader advertisement failed", "error", err) | ||
| } | ||
| c.heldHALock.Unlock() | ||
| c.heldHALock = nil | ||
| } | ||
|
|
||
| // Tell any requests that know about this to stop | ||
| if c.activeContextCancelFunc != nil { | ||
| c.activeContextCancelFunc() |
There was a problem hiding this comment.
Could you move this back where it was, it's designed to stop all pending requests prior to grabbing the lock.
There was a problem hiding this comment.
I think all this PR needs to be is a single line changing c.activeContext in clearLeader to context.Background().
There was a problem hiding this comment.
@briankassouf moved it back up - sorry I misunderstood.
@jefferai I like having ctx as the first argument to clearLeader, since that's "the Go way", but I updated it to use its own context.Background() instantiated just before the call.
|
@sethvargo it's only "the Go way" if you have a context you actually need to pass. In this case there isn't a context -- we're using the Background one because no other context makes sense. So it shouldn't be passed into clearLeader, because clearLeader doesn't need it. |
Move cancelation to after cleanup Fixes GH-4915
|
@jefferai okay updated |
|
👍 |
Move cancelation to after cleanup. This is my guess as to what's happening here, but I could be totally off base.
It looks like we call cancelation on
activeContext, but then pass that context to the function. I moved the cancelation to after cleanup, and made context a local param to reduce the chance of a race if another func modifies c.activeContext/cc @briankassouf @jefferai
Fixes GH-4915