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

Always Set Inprogress Boolean In Cache #13750

Merged
merged 1 commit into from
Mar 15, 2024
Merged

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Mar 15, 2024

What type of PR is this?

Bug Fix

What does this PR do? Why is it needed?

For our skip slot cache, we always set the in-progress boolean for a particular root regardless if it is enabled or disabled. This helps avoid wierd edge cases where the skip slot cache is disabled and then enabled which might lead to wierd deadlocks.

Which issues(s) does this PR fix?

Other notes for review

@nisdas nisdas added the Ready For Review A pull request ready for code review label Mar 15, 2024
@nisdas nisdas requested a review from a team as a code owner March 15, 2024 08:19
Copy link
Contributor

@saolyn saolyn left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

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

LGTM

@prestonvanloon prestonvanloon added this pull request to the merge queue Mar 15, 2024
Merged via the queue into develop with commit 58b8c31 Mar 15, 2024
17 checks passed
@prestonvanloon prestonvanloon deleted the alwaysMarkInProgress branch March 15, 2024 17:19
@PlasmaPower
Copy link

Shouldn't disabled also be made atomic? I don't think it's critical but in theory there's a race between writing and reading the disabled boolean.

@potuz
Copy link
Contributor

potuz commented Mar 15, 2024

I don't think there can be a race on reading these, as we first come out from round robin before starting to sync and it's only changed under locks I think.

@PlasmaPower
Copy link

I'd confirm that it is only changed under locks. I didn't see any relevant ones but I could've missed one. I'm also not sure how this issue would be hit if the relevant sections of code were under locks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants