Skip to content
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

stats: fix race condition when refreshing cache #51805

Merged
merged 1 commit into from
Jul 24, 2020

Conversation

rytaft
Copy link
Collaborator

@rytaft rytaft commented Jul 22, 2020

This commit fixes a race condition that was introduced in #51616.
The race condition existed because it was possible for two refreshes
to be triggered in close succession, and there was no mechanism to
prevent the first refresh from overwriting the second if it was
delayed.

We fix the race condition by introducing two boolean flags into each
cache entry: refreshing and mustRefreshAgain. If refreshing is true,
that indicates that the current statistics for this table are stale,
and we are in the process of fetching the updated stats from the database.
In the mean time, other callers can use the stale stats and do not need to
wait.

If a goroutine tries to perform a refresh when a refresh is already
in progress, it will see that refreshing=true and will set the
mustRefreshAgain flag to true before returning. When the original
goroutine that was performing the refresh returns from the database and
sees that mustRefreshAgain=true, it will trigger another refresh.

Informs #50863

Release note: None

@rytaft rytaft requested a review from RaduBerinde July 22, 2020 21:40
@rytaft rytaft requested a review from a team as a code owner July 22, 2020 21:40
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rytaft
Copy link
Collaborator Author

rytaft commented Jul 22, 2020

First commit is #51780

Copy link
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:

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

@rytaft
Copy link
Collaborator Author

rytaft commented Jul 23, 2020

TFTR!

bors r+

@rytaft
Copy link
Collaborator Author

rytaft commented Jul 23, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 23, 2020

Build failed:

@rytaft
Copy link
Collaborator Author

rytaft commented Jul 23, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 23, 2020

Build failed:

@rytaft
Copy link
Collaborator Author

rytaft commented Jul 23, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 23, 2020

Build failed:

This commit fixes a race condition that was introduced in cockroachdb#51616.
The race condition existed because it was possible for two refreshes
to be triggered in close succession, and there was no mechanism to
prevent the first refresh from overwriting the second if it was
delayed.

We fix the race condition by introducing two boolean flags into each
cache entry: refreshing and mustRefreshAgain. If refreshing is true,
that indicates that the current statistics for this table are stale,
and we are in the process of fetching the updated stats from the database.
In the mean time, other callers can use the stale stats and do not need to
wait.

If a goroutine tries to perform a refresh when a refresh is already
in progress, it will see that refreshing=true and will set the
mustRefreshAgain flag to true before returning. When the original
goroutine that was performing the refresh returns from the database and
sees that mustRefreshAgain=true, it will trigger another refresh.

Informs cockroachdb#50863

Release note: None
@rytaft
Copy link
Collaborator Author

rytaft commented Jul 23, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 23, 2020

Build failed (retrying...):

@jordanlewis
Copy link
Member

bors r+

@rytaft
Copy link
Collaborator Author

rytaft commented Jul 24, 2020

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 24, 2020

Build succeeded:

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.

4 participants