-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
tsdb/index/postings: fix missing lock unlock #14103
tsdb/index/postings: fix missing lock unlock #14103
Conversation
Followup to prometheus#14096 Unfortunately the previous PR introduced this bug by not releasing the lock before returning. Signed-off-by: György Krajcsovits <[email protected]>
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.
Could we just do defer p.mtx.RUnlock()
in line 402?
I suspect it's not done that way because of line 431, the last return |
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, right, we need to release it before executing Merge
. Too bad…
We could unlock by default in defer, and skip unlock in defer when not needed.
and set (We use this pattern in few places in Mimir.) |
Hmm, not sure what is less bad… it's both error prone, but the current solution is still a bit more compact (which would change if we had more exit points). |
grafana/mimir-prometheus@e5b85c1 To get prometheus/prometheus#14103 Signed-off-by: György Krajcsovits <[email protected]>
* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <[email protected]>
* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <[email protected]> (cherry picked from commit 307567a)
* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <[email protected]> (cherry picked from commit 307567a) Co-authored-by: George Krajcsovits <[email protected]>
* Update mimir-prometheus to e5b85c151ba8 grafana/mimir-prometheus@e5b85c1 prometheus/prometheus#14103 prometheus/prometheus#14096 prometheus/prometheus#14068 prometheus/prometheus#13669 Signed-off-by: György Krajcsovits <[email protected]>
Followup to #14096
Unfortunately the previous PR introduced this bug by not releasing the lock before returning.