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

Ledger: remove txtail from data race test #4315

Merged
merged 2 commits into from
Jul 28, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jul 27, 2022

Summary

The TestAcctUpdatesFastUpdates test calls committedUpTo and newBlock asynchronously with no locking, which causes a data race in the txtail. Since under normal operation these calls are protected by the tracker mutex, we can simply remove the txtail in this test to avoid test failure.

Test Plan

This is a test

@ghost ghost requested a review from algorandskiy July 27, 2022 21:14
@ghost ghost self-assigned this Jul 27, 2022
@@ -608,6 +608,9 @@ func TestAcctUpdatesFastUpdates(t *testing.T) {
defer au.close()
defer ao.close()

// remove the txtail from the list of trackers
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you included your explanation from the PR here?

@codecov
Copy link

codecov bot commented Jul 28, 2022

Codecov Report

Merging #4315 (fdf5621) into master (92e4f85) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #4315   +/-   ##
=======================================
  Coverage   55.25%   55.25%           
=======================================
  Files         395      395           
  Lines       50333    50333           
=======================================
  Hits        27814    27814           
+ Misses      20133    20130    -3     
- Partials     2386     2389    +3     
Impacted Files Coverage Δ
network/wsPeer.go 65.47% <0.00%> (-2.20%) ⬇️
util/db/dbutil.go 49.09% <0.00%> (-1.22%) ⬇️
catchup/peerSelector.go 98.95% <0.00%> (-1.05%) ⬇️
data/abi/abi_type.go 87.67% <0.00%> (-0.95%) ⬇️
catchup/service.go 69.62% <0.00%> (+0.24%) ⬆️
ledger/acctupdates.go 70.52% <0.00%> (+0.60%) ⬆️
ledger/tracker.go 78.87% <0.00%> (+1.72%) ⬆️
ledger/blockqueue.go 85.05% <0.00%> (+2.87%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

Copy link
Contributor

@algorandskiy algorandskiy left a comment

Choose a reason for hiding this comment

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

This is a right fix for this test because we do need to newBlock and committedUpTo to be concurrent in order to exercise another olded fixed 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.

3 participants