Skip to content

[FIXED] Avoid deadlock in updateAccountClaimsWithRefresh #6726

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

Merged
merged 2 commits into from
Mar 27, 2025

Conversation

MauriceVanVeen
Copy link
Member

Alternative fix for #6724, which could still deadlock.

Instead ensuring only one goroutine can go into this section in updateAccountClaimsWithRefresh which ensures we can't deadlock due to grabbing two account locks in succession.

Have placed the server lock before the range over the accounts, but it could optionally be moved to be around the acc.mu.Lock/Unlock instead.

Signed-off-by: Maurice van Veen [email protected]
Co-authored-by: Neil Twigg [email protected]

Signed-off-by: Maurice van Veen <[email protected]>
Co-authored-by: Neil Twigg <[email protected]>
@MauriceVanVeen MauriceVanVeen requested a review from a team as a code owner March 25, 2025 21:53
@MauriceVanVeen
Copy link
Member Author

The test is on the longer end in terms of runtime, roughly 30 seconds locally. Could move it to the long running tests, although those are primarily marked as JetStream cluster long running tests.

Signed-off-by: Maurice van Veen <[email protected]>
@derekcollison derekcollison requested a review from kozlovic March 25, 2025 22:04
Copy link
Member

@kozlovic kozlovic left a comment

Choose a reason for hiding this comment

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

I have checked the called functions and in none of them there is a case where we would lock the server, so using server lock here is ok. If there would be a need for getting the server lock somewhere in the stack, we would need a new mutex in the server object dedicated to this code (say updateClaimsMu).

Copy link
Member

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

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

LGTM

@neilalexander neilalexander merged commit ac23d77 into main Mar 27, 2025
37 checks passed
@neilalexander neilalexander deleted the maurice/claimrefresh branch March 27, 2025 09:14
@wallyqs wallyqs changed the title Avoid deadlock in updateAccountClaimsWithRefresh [FIXED] Avoid deadlock in updateAccountClaimsWithRefresh Apr 17, 2025
neilalexander added a commit that referenced this pull request Apr 17, 2025
Includes the following (already cherry-picked) PRs:

- #6587
- #6607
- #6612
- #6609
- #6620
- #6668
- #6674
- #6647
- #6684
- #6691
- #6697
- #6705
- #6706
- #6704
- #6714
- #6720
- #6727
- #6730
- #6726
- #6732
- #6759
- #6753
- #6685
- #6769
- #6777
- #6785
- #6786
- #6778
- #6790
- #6791
- #6798
- #6794
- #6801

Signed-off-by: Neil Twigg <[email protected]>

Signed-off-by: Neil Twigg <[email protected]>
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.

3 participants