Skip to content

Dispatch: Make sure mutex gets unlocked on call to Stop#2558

Merged
beorn7 merged 2 commits into
prometheus:masterfrom
grafana:bugfix/unlock-mutex
Apr 27, 2021
Merged

Dispatch: Make sure mutex gets unlocked on call to Stop#2558
beorn7 merged 2 commits into
prometheus:masterfrom
grafana:bugfix/unlock-mutex

Conversation

@aknuds1

@aknuds1 aknuds1 commented Apr 27, 2021

Copy link
Copy Markdown
Contributor

It looks as if Dispatch.Stop doesn't unlock the mutex if d.cancel == nil. I'm rectifying it to unlock it also if d.cancel == nil and the method returns early. It seems to fix a deadlock in Grafana (on server shutdown), during our test suite.

Here's a stacktrace of the involved goroutines when the deadlock occurs.

Let me know if you want me to write a test also.

aknuds1 added 2 commits April 27, 2021 09:25
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>
Signed-off-by: Arve Knudsen <arve.knudsen@gmail.com>

@pstibrany pstibrany left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM.

@stevesg

stevesg commented Apr 27, 2021

Copy link
Copy Markdown
Contributor

LGTM

@gotjosh gotjosh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@beorn7

beorn7 commented Apr 27, 2021

Copy link
Copy Markdown
Contributor

Interestingly, the unlock was correctly suggested in the review for the PR that introduced the bug, but then somehow slipped under the radar.

@beorn7 beorn7 merged commit 88f32c5 into prometheus:master Apr 27, 2021
@beorn7 beorn7 deleted the bugfix/unlock-mutex branch April 27, 2021 20:40
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.

6 participants