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

p2p: fixed data races #23434

Merged
merged 2 commits into from
Aug 24, 2021
Merged

p2p: fixed data races #23434

merged 2 commits into from
Aug 24, 2021

Conversation

MariusVanDerWijden
Copy link
Member

This PR fixes three data races found by the go race condition tester:

go test ./... --race
==================
WARNING: DATA RACE
Write at 0x00c00036f048 by goroutine 57:
  sync/atomic.CompareAndSwapInt32()
      /home/matematik/sdk/go1.14.7/src/runtime/race_amd64.s:293 +0xb
  github.com/ethereum/go-ethereum/p2p.(*conn).set()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/server.go:288 +0x68
  github.com/ethereum/go-ethereum/p2p.(*Server).run()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/server.go:725 +0x20d0

Previous read at 0x00c00036f048 by goroutine 32:
  github.com/ethereum/go-ethereum/p2p.(*dialScheduler).loop()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/dial.go:262 +0x1971

Goroutine 57 (running) created at:
  github.com/ethereum/go-ethereum/p2p.(*Server).Start()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/server.go:487 +0x740
  github.com/ethereum/go-ethereum/p2p.startTestServer()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/server_test.go:85 +0x22d
  github.com/ethereum/go-ethereum/p2p.TestServerDial()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/server_test.go:145 +0x269
  testing.tRunner()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1039 +0x1eb

Goroutine 32 (running) created at:
  github.com/ethereum/go-ethereum/p2p.newDialScheduler()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/dial.go:181 +0x67e
  github.com/ethereum/go-ethereum/p2p.(*Server).setupDialScheduler()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/server.go:633 +0x365
  github.com/ethereum/go-ethereum/p2p.(*Server).Start()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/server.go:484 +0x6fa
  github.com/ethereum/go-ethereum/p2p.startTestServer()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/server_test.go:85 +0x22d
  github.com/ethereum/go-ethereum/p2p.TestServerDial()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/server_test.go:145 +0x269
  testing.tRunner()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1039 +0x1eb
==================
--- FAIL: TestServerDial (0.00s)
    server.go:831: DEBUG[08-23|09:04:30.772] TCP listener up                          addr=127.0.0.1:39647
    dial.go:280: TRACE[08-23|09:04:30.772] Adding static node                       id=c5f29df73dcff2de ip=127.0.0.1 added=true
    dial.go:454: TRACE[08-23|09:04:30.773] Starting p2p dial                        id=c5f29df73dcff2de ip=127.0.0.1 flag=staticdial
    server.go:695: INFO [08-23|09:04:30.773] Started P2P networking                   self="enode://d0defda3b58ab67a7186b646f4765eaa5c5757dc8784780a13d171ae715dab57d5637db465a6947cd4e6f969be6bb4c52d1bc40a2b33b750e07a41dbeaa4466c@127.0.0.1:39647?discport=0"
    server.go:760: DEBUG[08-23|09:04:30.773] Adding p2p peer                          peercount=1 id=c5f29df73dcff2de conn=staticdial addr=127.0.0.1:45791 name=test
    server.go:722: TRACE[08-23|09:04:30.773] Adding trusted node                      node="enode://7de8876c2441d1d92b70e0d9d4347d4793a338ab6b84e1fb69e9e60f2a662c102fb260eb78856fed2cc6ea559b02bf354794ebe119bc56214b225c2229254eae@127.0.0.1:45791?discport=0"
    server.go:731: TRACE[08-23|09:04:30.774] Removing trusted node                    node="enode://7de8876c2441d1d92b70e0d9d4347d4793a338ab6b84e1fb69e9e60f2a662c102fb260eb78856fed2cc6ea559b02bf354794ebe119bc56214b225c2229254eae@127.0.0.1:45791?discport=0"
    server.go:871: DEBUG[08-23|09:04:30.774] Read error                               err="accept tcp 127.0.0.1:39647: use of closed network connection"
    server.go:780: TRACE[08-23|09:04:30.774] P2P networking is spinning down 
    server.go:798: TRACE[08-23|09:04:30.774] <-delpeer (spindown)                     id=c5f29df73dcff2de conn=staticdial
    testing.go:954: race detected during execution of test
FAIL
FAIL    github.com/ethereum/go-ethereum/p2p     2.101s
ok      github.com/ethereum/go-ethereum/p2p/discover    63.093s
ok      github.com/ethereum/go-ethereum/p2p/discover/v4wire     0.049s
ok      github.com/ethereum/go-ethereum/p2p/discover/v5wire     0.120s
ok      github.com/ethereum/go-ethereum/p2p/dnsdisc     0.913s
==================
WARNING: DATA RACE
Write at 0x00c000131158 by goroutine 13:
  github.com/ethereum/go-ethereum/p2p/enode.(*genIter).Close()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter_test.go:271 +0x3e
  github.com/ethereum/go-ethereum/p2p/enode.(*FairMix).Close()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter.go:193 +0x13b
  github.com/ethereum/go-ethereum/p2p/enode.testMixerFairness()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter_test.go:118 +0x56d
  github.com/ethereum/go-ethereum/p2p/enode.TestFairMix()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter_test.go:96 +0x41
  testing.tRunner()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1039 +0x1eb

Previous read at 0x00c000131158 by goroutine 14:
  sync/atomic.LoadInt32()
      /home/matematik/sdk/go1.14.7/src/runtime/race_amd64.s:206 +0xb
  github.com/ethereum/go-ethereum/p2p/enode.(*genIter).Next()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter_test.go:256 +0x42
  github.com/ethereum/go-ethereum/p2p/enode.(*FairMix).runSource()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter.go:279 +0x10e

Goroutine 13 (running) created at:
  testing.(*T).Run()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1090 +0x700
  testing.runTests.func1()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1334 +0xa6
  testing.tRunner()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1039 +0x1eb
  testing.runTests()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1332 +0x527
  testing.(*M).Run()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1249 +0x43f
  main.main()
      _testmain.go:104 +0x223

Goroutine 14 (running) created at:
  github.com/ethereum/go-ethereum/p2p/enode.(*FairMix).AddSource()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter.go:180 +0x2f1
  github.com/ethereum/go-ethereum/p2p/enode.testMixerFairness()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter_test.go:102 +0x212
  github.com/ethereum/go-ethereum/p2p/enode.TestFairMix()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter_test.go:96 +0x41
  testing.tRunner()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1039 +0x1eb
==================
--- FAIL: TestFairMix (5.96s)
    testing.go:954: race detected during execution of test
==================
WARNING: DATA RACE
Write at 0x00c000156058 by goroutine 18:
  github.com/ethereum/go-ethereum/p2p/enode.(*genIter).Close()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter_test.go:271 +0x3e
  github.com/ethereum/go-ethereum/p2p/enode.(*FairMix).Close()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter.go:193 +0x13b
  github.com/ethereum/go-ethereum/p2p/enode.TestFairMixNextFromAll()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter_test.go:135 +0x3ff
  testing.tRunner()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1039 +0x1eb

Previous read at 0x00c000156058 by goroutine 21:
  sync/atomic.LoadInt32()
      /home/matematik/sdk/go1.14.7/src/runtime/race_amd64.s:206 +0xb
  github.com/ethereum/go-ethereum/p2p/enode.(*genIter).Next()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter_test.go:256 +0x42
  github.com/ethereum/go-ethereum/p2p/enode.(*FairMix).runSource()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter.go:279 +0x10e

Goroutine 18 (running) created at:
  testing.(*T).Run()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1090 +0x700
  testing.runTests.func1()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1334 +0xa6
  testing.tRunner()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1039 +0x1eb
  testing.runTests()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1332 +0x527
  testing.(*M).Run()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1249 +0x43f
  main.main()
      _testmain.go:104 +0x223

Goroutine 21 (running) created at:
  github.com/ethereum/go-ethereum/p2p/enode.(*FairMix).AddSource()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter.go:180 +0x2f1
  github.com/ethereum/go-ethereum/p2p/enode.TestFairMixNextFromAll()
      /home/matematik/go/src/github.com/ethereum/go-ethereum/p2p/enode/iter_test.go:124 +0x20f
  testing.tRunner()
      /home/matematik/sdk/go1.14.7/src/testing/testing.go:1039 +0x1eb
==================
--- FAIL: TestFairMixNextFromAll (0.02s)
    testing.go:954: race detected during execution of test
FAIL
FAIL    github.com/ethereum/go-ethereum/p2p/enode       6.076s
ok      github.com/ethereum/go-ethereum/p2p/enr 0.028s
ok      github.com/ethereum/go-ethereum/p2p/msgrate     0.046s
ok      github.com/ethereum/go-ethereum/p2p/nat 2.652s
ok      github.com/ethereum/go-ethereum/p2p/netutil     0.077s
ok      github.com/ethereum/go-ethereum/p2p/nodestate   0.437s
ok      github.com/ethereum/go-ethereum/p2p/rlpx        0.048s
ok      github.com/ethereum/go-ethereum/p2p/simulations 3.249s
ok      github.com/ethereum/go-ethereum/p2p/simulations/adapters        0.072s
?       github.com/ethereum/go-ethereum/p2p/simulations/examples        [no test files]
?       github.com/ethereum/go-ethereum/p2p/simulations/pipes   [no test files]
?       github.com/ethereum/go-ethereum/p2p/tracker     [no test files]
FAIL

@MariusVanDerWijden MariusVanDerWijden mentioned this pull request Aug 23, 2021
Closed
@fjl fjl merged commit 8dbf261 into ethereum:master Aug 24, 2021
@fjl fjl added this to the 1.10.9 milestone Aug 24, 2021
sidhujag pushed a commit to sidhujag/go-ethereum that referenced this pull request Aug 24, 2021
@MariusVanDerWijden @fjl
p2p, p2p/enode: fix data races (ethereum#23434)
8277a18 
In p2p/dial.go, conn.flags was accessed without using sync/atomic.
This race is fixed by removing the access.

In p2p/enode/iter_test.go, a similar race is resolved by writing the field atomically.

Co-authored-by: Felix Lange <fjl@twurst.com>
@MariusVanDerWijden MariusVanDerWijden mentioned this pull request Aug 27, 2021
Merged
@MariusVanDerWijden MariusVanDerWijden deleted the p2p-race branch November 30, 2021 15:26
This was referenced Sep 23, 2022
Closed
Merged
yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
@MariusVanDerWijden @fjl @zzyalbert
p2p, p2p/enode: fix data races (ethereum#23434)
63461d8 
In p2p/dial.go, conn.flags was accessed without using sync/atomic.
This race is fixed by removing the access.

In p2p/enode/iter_test.go, a similar race is resolved by writing the field atomically.

Co-authored-by: Felix Lange <fjl@twurst.com>
nibty pushed a commit to FairCrypto/go-ethereum that referenced this pull request Apr 10, 2024
@MariusVanDerWijden @fjl @nibty
p2p, p2p/enode: fix data races (ethereum#23434)
a8123e6 
In p2p/dial.go, conn.flags was accessed without using sync/atomic.
This race is fixed by removing the access.

In p2p/enode/iter_test.go, a similar race is resolved by writing the field atomically.

Co-authored-by: Felix Lange <fjl@twurst.com>
nibty pushed a commit to FairCrypto/go-ethereum that referenced this pull request Apr 10, 2024
@MariusVanDerWijden @fjl @nibty
p2p, p2p/enode: fix data races (ethereum#23434)
ef23056 
In p2p/dial.go, conn.flags was accessed without using sync/atomic.
This race is fixed by removing the access.

In p2p/enode/iter_test.go, a similar race is resolved by writing the field atomically.

Co-authored-by: Felix Lange <fjl@twurst.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fjl fjl fjl approved these changes

@zsfelfoldi zsfelfoldi Awaiting requested review from zsfelfoldi zsfelfoldi is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.10.9
Development

Successfully merging this pull request may close these issues.
2 participants
@MariusVanDerWijden @fjl