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 race condition in dispatcher #2208
Fix race condition in dispatcher #2208
Conversation
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. Would you mind adding the test that you wrote in the original issue?
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
95576db
to
8d28473
Compare
@simonpasquier I added the test from the original issue. Still not passing the CI test cases but the failures look unrelated to this PR. |
Thanks! The CI is a bit flaky sometimes. I've restarted it and will merge on green. |
@@ -255,11 +254,13 @@ func (d *Dispatcher) Groups(routeFilter func(*Route) bool, alertFilter func(*typ | |||
|
|||
// Stop the dispatcher. | |||
func (d *Dispatcher) Stop() { | |||
d.mtx.Lock() |
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.
Ah the failure is legit! Alertmanager crashes on startup because the Stop()
function is called with a nil
receiver. We need this:
if d == nil {
return
}
d.mtx.Lock()
if d.cancel == nil {
d.mtx.Unlock()
return nil
}
...
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Signed-off-by: Jacob Lisi <jacob.t.lisi@gmail.com>
Thanks! |
Fixes: #2182
By ensuring operations on the
cancel
andctx
field are wrapped with a mutex this removes the race condition from the dispatcher.