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.

Sign up for GitHub

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

Jump to bottom

logger: fix data race in tests #5999

Merged
merged 1 commit into from
May 14, 2024
Merged

logger: fix data race in tests #5999

merged 1 commit into from
May 14, 2024

Conversation

algorandskiy
Copy link
Contributor

@algorandskiy algorandskiy commented May 13, 2024 •
edited
Loading

Summary

Fix for the following data race seen in tests:

=== RUN   TestVoteAggregatorFiltersBundlePresent
==================
WARNING: DATA RACE
Read at 0x00c0005e3260 by goroutine 483304:
  github.com/sirupsen/logrus.(*Entry).write()
      /opt/cibuild/go/pkg/mod/github.com/sirupsen/logrus@v1.8.1/entry.go:286 +0x23e
  github.com/sirupsen/logrus.(*Entry).log()
      /opt/cibuild/go/pkg/mod/github.com/sirupsen/logrus@v1.8.1/entry.go:251 +0x964
  github.com/sirupsen/logrus.(*Entry).Log()
      /opt/cibuild/go/pkg/mod/github.com/sirupsen/logrus@v1.8.1/entry.go:293 +0x8b
  github.com/sirupsen/logrus.(*Entry).Logf()
      /opt/cibuild/go/pkg/mod/github.com/sirupsen/logrus@v1.8.1/entry.go:338 +0xd8
  github.com/sirupsen/logrus.(*Entry).Infof()
      /opt/cibuild/go/pkg/mod/github.com/sirupsen/logrus@v1.8.1/entry.go:351 +0x86
  github.com/algorand/go-algorand/logging.logger.Infof()
      /opt/cibuild/project/logging/log.go:205 +0x4f
  github.com/algorand/go-algorand/logging.(*logger).Infof()
      <autogenerated>:1 +0x84
  github.com/algorand/go-algorand/agreement.pseudonodeVotesTask.execute()

Previous write at 0x00c0005e3260 by goroutine 493961:
  github.com/algorand/go-algorand/logging.logger.setOutput()
      /opt/cibuild/project/logging/log.go:311 +0x1bb
  github.com/algorand/go-algorand/logging.logger.SetOutput()
      /opt/cibuild/project/logging/log.go:306 +0x1a2
  github.com/algorand/go-algorand/logging.(*logger).SetOutput()
      <autogenerated>:1 +0x24
  github.com/algorand/go-algorand/agreement.(*determisticTraceTestCase).ValidateAsExtension.func1()
      /opt/cibuild/project/agreement/state_machine_test.go:417 +0x6f
  runtime.deferreturn()
      /opt/cibuild/.gimme/versions/go1.21.10.linux.amd64/src/runtime/panic.go:477 +0x30
  github.com/algorand/go-algorand/agreement.(*directMatchIoSafetyProp).containsTrace()
      /opt/cibuild/project/agreement/state_machine_test.go:255 +0xc1

Logger.SetOutput takes lock before assigning Logger.Out that is a correct thing to do.

Test Plan

Existing tests should pass.

@algorandskiy algorandskiy requested review from cce and gmalouf May 13, 2024 16:20
@algorandskiy algorandskiy self-assigned this May 13, 2024
@codecov Codecov
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.92%. Comparing base (6c91f52) to head (fc7e081).
Report is 5 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5999      +/-   ##
==========================================
- Coverage   55.94%   55.92%   -0.03%     
==========================================
  Files         482      482              
  Lines       68310    68310              
==========================================
- Hits        38219    38204      -15     
- Misses      27500    27510      +10     
- Partials     2591     2596       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jannotti jannotti merged commit 299b309 into algorand:master May 14, 2024
20 checks passed
This was referenced Jun 7, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmalouf gmalouf gmalouf approved these changes

@jannotti jannotti jannotti approved these changes

@cce cce Awaiting requested review from cce

Assignees

@algorandskiy algorandskiy

Labels
Bug-Fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@algorandskiy @jannotti @gmalouf