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

race condition: Add lock around catchpointsMu to avoid race condition #3944

Merged
merged 1 commit into from
May 3, 2022

Conversation

Eric-Warehime
Copy link
Contributor

Summary

While upgrading to golang 1.17.9 I've run into a couple of race conditions which have been detected during E2E tests. This fixes one of them during which a write happens at https://github.com/algorand/go-algorand/blob/master/ledger/catchpointtracker.go#L417 and a read happens at https://github.com/algorand/go-algorand/blob/master/ledger/catchpointtracker.go#L173

Test Plan

e2e tests

@Eric-Warehime Eric-Warehime changed the title Add lock around catchpointsMu to avoid race condition race condition: Add lock around catchpointsMu to avoid race condition May 2, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #3944 (8052cb4) into master (cfe6e57) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #3944   +/-   ##
=======================================
  Coverage   49.97%   49.98%           
=======================================
  Files         400      400           
  Lines       68633    68635    +2     
=======================================
+ Hits        34301    34305    +4     
- Misses      30630    30631    +1     
+ Partials     3702     3699    -3     
Impacted Files Coverage Δ
ledger/catchpointtracker.go 57.06% <100.00%> (+0.16%) ⬆️
ledger/blockqueue.go 82.18% <0.00%> (-2.88%) ⬇️
catchup/service.go 68.14% <0.00%> (-1.24%) ⬇️
data/transactions/verify/txn.go 44.15% <0.00%> (-0.87%) ⬇️
cmd/tealdbg/debugger.go 72.69% <0.00%> (-0.81%) ⬇️
ledger/acctupdates.go 69.30% <0.00%> (+0.79%) ⬆️
network/wsPeer.go 71.66% <0.00%> (+2.77%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfe6e57...8052cb4. Read the comment docs.

@Eric-Warehime Eric-Warehime marked this pull request as ready for review May 2, 2022 17:15
@algorandskiy algorandskiy requested a review from cce May 2, 2022 18:39
algorandskiy
algorandskiy previously approved these changes May 2, 2022
@@ -414,7 +414,9 @@ func (ct *catchpointTracker) postCommit(ctx context.Context, dcc *deferredCommit
}

if dcc.isCatchpointRound && dcc.catchpointLabel != "" {
ct.catchpointsMu.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

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

This same lock is taken to update ct.roundDigest below, @algorandskiy do you think both ct.roundDigest and ct.lastCatchpointLabel need to be updated in the same locked section?

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this is not necessary since we generate it every 10k rounds, and not every round.
But for consistency I agree, it is better to move this if statement under the lock below.
@Eric-Warehime could you please do that?

@cce
Copy link
Contributor

cce commented May 2, 2022

@algorandskiy for historical purposes I looked it up in an old release before the catchpointtracker was introduced, like 3.0.1, and au.lastCatchpointLabel is updated just before au.accountsMu.Lock() too

https://github.com/algorand/go-algorand/blob/v3.0.1-stable/ledger/acctupdates.go#L2265
https://github.com/algorand/go-algorand/blob/v3.0.1-stable/ledger/acctupdates.go#L644

algorandskiy
algorandskiy previously approved these changes May 2, 2022
@algorandskiy algorandskiy merged commit cd4015a into algorand:master May 3, 2022
@Eric-Warehime Eric-Warehime deleted the catchpoint-lock branch May 3, 2022 06:20
@tolikzinovyev
Copy link
Contributor

FYI, catchpointTracker.accountDataResourceSeparationRound is not protected my a mutex in all places -- another data race.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants