Skip to content

metric: fix missing mutex handling in metrics registry#69538

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
adityamaru:missing-mutex
Aug 29, 2021
Merged

metric: fix missing mutex handling in metrics registry#69538
craig[bot] merged 1 commit intocockroachdb:masterfrom
adityamaru:missing-mutex

Conversation

@adityamaru
Copy link
Copy Markdown
Contributor

@adityamaru adityamaru commented Aug 28, 2021

This change ensures that the metrics registry locks and unlocks
whenever accessing its shared fields.

Fixes: #69522

Release note: None

Release justification: bug fixes and low-risk updates to new functionality

@adityamaru adityamaru requested a review from knz August 28, 2021 18:49
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@adityamaru
Copy link
Copy Markdown
Contributor Author

I think we just forgot to lock in some places. I tested TestPlainHTTPServer under race and it no longer fails.

Copy link
Copy Markdown
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @knz)

Copy link
Copy Markdown
Member

@RaduBerinde RaduBerinde left a comment

Choose a reason for hiding this comment

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

Hm, the metric package timed out, deadlock?

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @knz)

@adityamaru
Copy link
Copy Markdown
Contributor Author

doh, yeah it's the change in the test file, the methods that invoke findMetricByName already hold a lock. Let me fix it.

This change ensures that the registry locks and unlocks
whenever accessing its shared fields.

Fixes: cockroachdb#69522

Release note: None

Release justification: bug fixes and low-risk updates to new functionality
@RaduBerinde
Copy link
Copy Markdown
Member

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 29, 2021

Build succeeded:

@craig craig bot merged commit 1587b7d into cockroachdb:master Aug 29, 2021
@adityamaru adityamaru deleted the missing-mutex branch August 29, 2021 01:03
@pbardea
Copy link
Copy Markdown
Contributor

pbardea commented Aug 30, 2021

@adityamaru does this need a backport as well?

@adityamaru
Copy link
Copy Markdown
Contributor Author

Yeah you're right, it does. Labelling it so that I can open one once this bakes.

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.

server: race condition between metric recorder and job scheduler initialization

4 participants