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

draft: test data race and postMessagesOfInterestThread #5412

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

algonautshant
Copy link
Contributor

@algonautshant algonautshant commented May 23, 2023

This PR is to address the following race condition below.

This fix is to replacelogging.TestingLog with logging.Base.
When the network stops, there will be some associated services that will shut down asynchronously. Thus, they may still log something after the test ends, and that is not allowed. It will either panic or trigger the data race detector.

In the process of fixing this, I noticed that the postMessagesOfInterestThread is not terminated after stopping the network. A draft fix for it is also here.

https://circleci.com/api/v1.1/project/github/algorand/go-algorand/234612/output/114/17?file=true&allocation-id=646b8c40d9ef8372d2bb808f-17-build%2FEUE0509X

WARNING: DATA RACE
Write at 0x00c000093be3 by goroutine 53:
  testing.tRunner.func1()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:1246 +0x584
  testing.tRunner()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:1265 +0x268
  testing.(*T).Run·dwrap·21()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:1306 +0x47

Previous read at 0x00c000093be3 by goroutine 1787:
  testing.(*common).logDepth()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:768 +0xc4
  testing.(*common).log()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:761 +0x5a
  testing.(*common).Log()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:800 +0x14
  testing.(*T).Log()
      <autogenerated>:1 +0x55
  github.com/algorand/go-algorand/logging.TestLogWriter.Write()
      /opt/cibuild/project/logging/testingLogger.go:38 +0x107
  github.com/algorand/go-algorand/logging.(*TestLogWriter).Write()
      <autogenerated>:1 +0x7a
  github.com/sirupsen/logrus.(*Entry).write()
      /opt/cibuild/go/pkg/mod/github.com/sirupsen/[email protected]/entry.go:286 +0x258
  github.com/sirupsen/logrus.(*Entry).log()
      /opt/cibuild/go/pkg/mod/github.com/sirupsen/[email protected]/entry.go:251 +0x453
  github.com/sirupsen/logrus.(*Entry).Log()
      /opt/cibuild/go/pkg/mod/github.com/sirupsen/[email protected]/entry.go:293 +0x8b
  github.com/sirupsen/logrus.(*Entry).Logf()
      /opt/cibuild/go/pkg/mod/github.com/sirupsen/[email protected]/entry.go:338 +0xc4
  github.com/sirupsen/logrus.(*Entry).Infof()
      /opt/cibuild/go/pkg/mod/github.com/sirupsen/[email protected]/entry.go:351 +0x8d
  github.com/algorand/go-algorand/logging.logger.Infof()
      /opt/cibuild/project/logging/log.go:205 +0x2c
  github.com/algorand/go-algorand/logging.(*logger).Infof()
      <autogenerated>:1 +0x93
  github.com/algorand/go-algorand/network.(*wsPeer).Close()
      /opt/cibuild/project/network/wsPeer.go:886 +0x2f8
  github.com/algorand/go-algorand/network.(*wsPeer).internalClose()
      /opt/cibuild/project/network/wsPeer.go:876 +0xd7
  github.com/algorand/go-algorand/network.(*wsPeer).readLoopCleanup()
      /opt/cibuild/project/network/wsPeer.go:659 +0x51
  github.com/algorand/go-algorand/network.(*wsPeer).readLoop.func1()
      /opt/cibuild/project/network/wsPeer.go:473 +0x64
  github.com/algorand/go-algorand/network.(*wsPeer).readLoop()
      /opt/cibuild/project/network/wsPeer.go:488 +0x1c2d
  github.com/algorand/go-algorand/network.(*wsPeer).init·dwrap·69()
      /opt/cibuild/project/network/wsPeer.go:446 +0x39

Goroutine 53 (running) created at:
  testing.(*T).Run()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:1306 +0x726
  testing.runTests.func1()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:1598 +0x99
  testing.tRunner()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:1259 +0x22f
  testing.runTests()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:1596 +0x7ca
  testing.(*M).Run()
      /opt/cibuild/.gimme/versions/go1.17.13.linux.amd64/src/testing/testing.go:1504 +0x9d1
  github.com/algorand/go-algorand/network.TestMain()
      /opt/cibuild/project/network/wsNetwork_test.go:68 +0x56
  main.main()
      _testmain.go:405 +0x364

Goroutine 1787 (finished) created at:
  github.com/algorand/go-algorand/network.(*wsPeer).init()
      /opt/cibuild/project/network/wsPeer.go:446 +0x704
  github.com/algorand/go-algorand/network.(*WebsocketNetwork).tryConnect()
      /opt/cibuild/project/network/wsNetwork.go:2315 +0x1c64
  github.com/algorand/go-algorand/network.(*WebsocketNetwork).checkNewConnectionsNeeded·dwrap·53()
      /opt/cibuild/project/network/wsNetwork.go:1776 +0x71

…ter network stop, use reg log instead of test log
@algonautshant algonautshant changed the title tag pos log after network shutdown, terminate goroutine not closed af… draft: test data race and postMessagesOfInterestThread May 23, 2023
Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 53.34%. Comparing base (99081cb) to head (8ceaaf4).
Report is 554 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5412      +/-   ##
==========================================
- Coverage   55.41%   53.34%   -2.08%     
==========================================
  Files         452      452              
  Lines       63826    63829       +3     
==========================================
- Hits        35369    34049    -1320     
- Misses      26028    27330    +1302     
- Partials     2429     2450      +21     

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

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.

2 participants