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

Fix race on BasePeerLeecher.done #77

Merged
merged 1 commit into from
Jan 16, 2024
Merged

Fix race on BasePeerLeecher.done #77

merged 1 commit into from
Jan 16, 2024

Conversation

thaarok
Copy link
Contributor

@thaarok thaarok commented Jan 8, 2024

BasePeerLeecher currently allows race conditions on done boolean field. It reads it in the loop() method without locking any mutex.

A trivial fix would be to add quitMu locking around done reading, however that would lock on every leecher tick, which can lead to performance drop down. From that reason, I have replaced the quitMu mutex with atomic integer instead.

Race condition details:

==================
WARNING: DATA RACE
Write at 0x00c0fabfc720 by goroutine 2181:
  github.com/Fantom-foundation/lachesis-base/gossip/basestream/basestreamleecher/basepeerleecher.(*BasePeerLeecher).Terminate()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/gossip/basestream/basestreamleecher/basepeerleecher/session.go:85 +0xc8
  github.com/Fantom-foundation/go-opera/gossip/protocols/dag/dagstream/dagstreamleecher.(*Leecher).terminateSession()
      /home/jand/go-opera-norma/gossip/protocols/dag/dagstream/dagstreamleecher/leecher.go:87 +0x64
  github.com/Fantom-foundation/go-opera/gossip/protocols/dag/dagstream/dagstreamleecher.(*Leecher).NotifyChunkReceived()
      /home/jand/go-opera-norma/gossip/protocols/dag/dagstream/dagstreamleecher/leecher.go:223 +0x404
  github.com/Fantom-foundation/go-opera/gossip.(*handler).handleMsg()
      /home/jand/go-opera-norma/gossip/handler.go:1180 +0x54fe
  github.com/Fantom-foundation/go-opera/gossip.(*handler).handle()
      /home/jand/go-opera-norma/gossip/handler.go:852 +0x1bec
  github.com/Fantom-foundation/go-opera/gossip.MakeProtocols.func1()
      /home/jand/go-opera-norma/gossip/service.go:373 +0x1ea
  github.com/ethereum/go-ethereum/p2p.(*Peer).startProtocols.func1()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/p2p/peer.go:407 +0xf4

Previous read at 0x00c0fabfc720 by goroutine 5834:
  github.com/Fantom-foundation/lachesis-base/gossip/basestream/basestreamleecher/basepeerleecher.(*BasePeerLeecher).loop()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/gossip/basestream/basestreamleecher/basepeerleecher/session.go:118 +0x204
  github.com/Fantom-foundation/lachesis-base/gossip/basestream/basestreamleecher/basepeerleecher.(*BasePeerLeecher).Start.func1()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/gossip/basestream/basestreamleecher/basepeerleecher/session.go:69 +0x79

Goroutine 2181 (running) created at:
  github.com/ethereum/go-ethereum/p2p.(*Peer).startProtocols()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/p2p/peer.go:405 +0x111
  github.com/ethereum/go-ethereum/p2p.(*Peer).run()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/p2p/peer.go:250 +0x219
  github.com/ethereum/go-ethereum/p2p.(*Server).runPeer()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/p2p/server.go:1081 +0x373
  github.com/ethereum/go-ethereum/p2p.(*Server).launchPeer.func1()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/p2p/server.go:1064 +0x44

Goroutine 5834 (running) created at:
  github.com/Fantom-foundation/lachesis-base/gossip/basestream/basestreamleecher/basepeerleecher.(*BasePeerLeecher).Start()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/gossip/basestream/basestreamleecher/basepeerleecher/session.go:67 +0xbc
  github.com/Fantom-foundation/go-opera/gossip/protocols/dag/dagstream/dagstreamleecher.(*Leecher).startSession()
      /home/jand/go-opera-norma/gossip/protocols/dag/dagstream/dagstreamleecher/leecher.go:183 +0xc46
  github.com/Fantom-foundation/go-opera/gossip/protocols/dag/dagstream/dagstreamleecher.(*Leecher).startSession-fm()
      <autogenerated>:1 +0x51
  github.com/Fantom-foundation/lachesis-base/gossip/basestream/basestreamleecher.(*BaseLeecher).Routine()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/gossip/basestream/basestreamleecher/base_leecher.go:64 +0x124
  github.com/Fantom-foundation/lachesis-base/gossip/basestream/basestreamleecher.(*BaseLeecher).loop()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/gossip/basestream/basestreamleecher/base_leecher.go:78 +0xf1
  github.com/Fantom-foundation/lachesis-base/gossip/basestream/basestreamleecher.(*BaseLeecher).Start.func1()
      /home/jand/go/pkg/mod/github.com/!fantom-foundation/[email protected]/gossip/basestream/basestreamleecher/base_leecher.go:50 +0x68
==================

@thaarok thaarok requested a review from andrecronje as a code owner January 8, 2024 18:45
Copy link

@jmpike jmpike 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, as long as the package doesn't touch .done and/or .quit directly and uses only the Terminate() and Stopped() interface.

Thank you, Honza.

@uprendis uprendis merged commit a75735c into Fantom-foundation:develop Jan 16, 2024
6 checks passed
@thaarok thaarok deleted the jkalina/race4-fix branch January 16, 2024 09:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants