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

tests: fix close - commit data race in tracker tests #5619

Merged

Conversation

algorandskiy
Copy link
Contributor

Summary

Fixed data race from this build:

WARNING: DATA RACE
Read at 0x00c000144900 by goroutine 3010:
  runtime.mapaccess1()
      /opt/cibuild/.gimme/versions/go1.20.5.linux.amd64/src/runtime/map.go:395 +0x0
  github.com/algorand/go-algorand/ledger.(*lruAccounts).read()
      /opt/cibuild/project/ledger/lruaccts.go:66 +0x97
  github.com/algorand/go-algorand/ledger.makeCompactAccountDeltas()
      /opt/cibuild/project/ledger/acctdeltas.go:453 +0x878
  github.com/algorand/go-algorand/ledger.(*accountUpdates).prepareCommit()
      /opt/cibuild/project/ledger/acctupdates.go:1555 +0x688
  github.com/algorand/go-algorand/ledger.(*trackerRegistry).commitRound()
      /opt/cibuild/project/ledger/tracker.go:538 +0x641
  github.com/algorand/go-algorand/ledger.(*trackerRegistry).commitSyncer()
      /opt/cibuild/project/ledger/tracker.go:480 +0x20d
  github.com/algorand/go-algorand/ledger.(*trackerRegistry).initialize.func2()
      /opt/cibuild/project/ledger/tracker.go:308 +0x47

Previous write at 0x00c000144900 by goroutine 2978:
  runtime.mapdelete()
      /opt/cibuild/.gimme/versions/go1.20.5.linux.amd64/src/runtime/map.go:695 +0x0
  github.com/algorand/go-algorand/ledger.(*lruAccounts).prune()
      /opt/cibuild/project/ledger/lruaccts.go:163 +0x113
  github.com/algorand/go-algorand/ledger.(*accountUpdates).close()
      /opt/cibuild/project/ledger/acctupdates.go:323 +0xee
  github.com/algorand/go-algorand/ledger.testAcctUpdatesUpdatesCorrectness.func1.2()
      /opt/cibuild/project/ledger/acctupdates_test.go:801 +0x39
  runtime.deferCallSave()
      /opt/cibuild/.gimme/versions/go1.20.5.linux.amd64/src/runtime/panic.go:796 +0x87
  testing.(*T).FailNow()
      <autogenerated>:1 +0x37
  github.com/stretchr/testify/require.Equalf()
      /opt/cibuild/go/pkg/mod/github.com/stretchr/testify@v1.8.4/require/require.go:280 +0x12d
  github.com/algorand/go-algorand/ledger.testAcctUpdatesUpdatesCorrectness.func1()
      /opt/cibuild/project/ledger/acctupdates_test.go:872 +0x2524

Test Plan

Existing tests

@codecov
Copy link

codecov bot commented Jul 28, 2023

Codecov Report

Merging #5619 (1423938) into master (1bf7a21) will increase coverage by 0.03%.
Report is 8 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5619      +/-   ##
==========================================
+ Coverage   54.95%   54.98%   +0.03%     
==========================================
  Files         463      463              
  Lines       64525    64525              
==========================================
+ Hits        35457    35480      +23     
+ Misses      26684    26664      -20     
+ Partials     2384     2381       -3     

see 15 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@algorandskiy algorandskiy marked this pull request as draft July 29, 2023 01:00
@algorandskiy algorandskiy changed the title ledger: fix close - commit data race WIP: tests: fix close - commit data race in tracker tests Jul 29, 2023
@algorandskiy
Copy link
Contributor Author

same data race on nightly

@algorandskiy algorandskiy marked this pull request as ready for review July 29, 2023 02:09
@algorandskiy algorandskiy changed the title WIP: tests: fix close - commit data race in tracker tests tests: fix close - commit data race in tracker tests Jul 29, 2023
@algorandskiy algorandskiy force-pushed the pavel/acct-updates-race-fix branch 2 times, most recently from 45fe10f to 2ec26f6 Compare July 31, 2023 13:46
algonautshant
algonautshant previously approved these changes Aug 3, 2023
Copy link
Contributor

@algonautshant algonautshant left a comment

Choose a reason for hiding this comment

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

Looks good. Just trivial fix suggestion for comments.

ledger/acctupdates_test.go Outdated Show resolved Hide resolved
ledger/acctupdates_test.go Outdated Show resolved Hide resolved
ledger/acctupdates_test.go Outdated Show resolved Hide resolved
ledger/acctupdates_test.go Outdated Show resolved Hide resolved
ledger/acctupdates_test.go Outdated Show resolved Hide resolved
ledger/acctupdates_test.go Outdated Show resolved Hide resolved
ledger/acctupdates_test.go Outdated Show resolved Hide resolved
ledger/voters_test.go Outdated Show resolved Hide resolved
ledger/voters_test.go Show resolved Hide resolved
ledger/voters_test.go Outdated Show resolved Hide resolved
@cce
Copy link
Contributor

cce commented Aug 3, 2023

What was merged that caused this race?

@algorandskiy
Copy link
Contributor Author

#5527 made it much more observable.

Co-authored-by: Shant Karakashian <55754073+algonautshant@users.noreply.github.com>
algonautshant
algonautshant previously approved these changes Aug 3, 2023
ledger/voters_test.go Outdated Show resolved Hide resolved
@algorandskiy algorandskiy merged commit fb8adcf into algorand:master Aug 9, 2023
18 checks passed
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.

None yet

4 participants