search

ipfs / kubo

An IPFS implementation in Go
https://docs.ipfs.tech/how-to/command-line-quick-start/
Other
16k stars 3k forks source link

Race condition #4105

Closed JustinDrake closed 1 year ago

JustinDrake commented 7 years ago

I building on top of go-ipfs and I have a race condition in my program. It turns out the race is part of the go-ipfs dependency, and can easily be triggered with make test_go_race on my system (MacOS X). I am trying to get my programs race-free so any help fixing it would be greatly appreciated.

Below is the race report.

==================
WARNING: DATA RACE
Write at 0x00c4202d54b0 by goroutine 118:
  gx/ipfs/QmTnsezaB1wWNRHeHnYrm8K4d5i9wtyj3GsqjC3Rt5b5v5/go-multistream.(*MultistreamMuxer).NegotiateLazy.func1()
      /Users/justin/go/src/gx/ipfs/QmTnsezaB1wWNRHeHnYrm8K4d5i9wtyj3GsqjC3Rt5b5v5/go-multistream/multistream.go:189 +0x358

Previous read at 0x00c4202d54b0 by goroutine 40:
  gx/ipfs/QmTnsezaB1wWNRHeHnYrm8K4d5i9wtyj3GsqjC3Rt5b5v5/go-multistream.(*lazyConn).Write()
      /Users/justin/go/src/gx/ipfs/QmTnsezaB1wWNRHeHnYrm8K4d5i9wtyj3GsqjC3Rt5b5v5/go-multistream/lazy.go:114 +0x48
  gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic.(*streamWrapper).Write()
      /Users/justin/go/src/gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic/basic_host.go:462 +0x75
  gx/ipfs/QmZ4Qi3GaRbjcx28Sme5eMH7RQjGkt8wHxt2a65oLaeFEV/gogo-protobuf/io.(*varintWriter).WriteMsg()
      /Users/justin/go/src/gx/ipfs/QmZ4Qi3GaRbjcx28Sme5eMH7RQjGkt8wHxt2a65oLaeFEV/gogo-protobuf/io/varint.go:74 +0x259
  gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/protocol/identify.(*IDService).RequestHandler()
      /Users/justin/go/src/gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/protocol/identify/id.go:133 +0x360
  gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/protocol/identify.(*IDService).RequestHandler-fm()
      /Users/justin/go/src/gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/protocol/identify/id.go:65 +0x55
  gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic.(*BasicHost).SetStreamHandler.func1()
      /Users/justin/go/src/gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic/basic_host.go:263 +0xb4

Goroutine 118 (running) created at:
  gx/ipfs/QmTnsezaB1wWNRHeHnYrm8K4d5i9wtyj3GsqjC3Rt5b5v5/go-multistream.(*MultistreamMuxer).NegotiateLazy()
      /Users/justin/go/src/gx/ipfs/QmTnsezaB1wWNRHeHnYrm8K4d5i9wtyj3GsqjC3Rt5b5v5/go-multistream/multistream.go:190 +0x222
  gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic.(*BasicHost).newStreamHandler()
      /Users/justin/go/src/gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic/basic_host.go:191 +0x125
  gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic.(*BasicHost).(gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic.newStreamHandler)-fm()
      /Users/justin/go/src/gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic/basic_host.go:142 +0x55
  gx/ipfs/QmaijwHnbD4SabGA8C2fN9gchptLvRe2RxqTU5XkjAGBw5/go-libp2p-swarm.(*Swarm).SetStreamHandler.func1()
      /Users/justin/go/src/gx/ipfs/QmaijwHnbD4SabGA8C2fN9gchptLvRe2RxqTU5XkjAGBw5/go-libp2p-swarm/swarm.go:269 +0x52
  gx/ipfs/Qma887khroMXGLJuHLYqqDZXHivAfFPxd2hQ8Z5kucMWTM/go-peerstream.(*Swarm).addConn.func1()
      /Users/justin/go/src/gx/ipfs/Qma887khroMXGLJuHLYqqDZXHivAfFPxd2hQ8Z5kucMWTM/go-peerstream/conn.go:213 +0x92

Goroutine 40 (running) created at:
  gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic.(*BasicHost).newStreamHandler()
      /Users/justin/go/src/gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic/basic_host.go:227 +0xafe
  gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic.(*BasicHost).(gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic.newStreamHandler)-fm()
      /Users/justin/go/src/gx/ipfs/QmapADMpK4e5kFGBxC2aHreaDqKP9vmMng5f91MA14Ces9/go-libp2p/p2p/host/basic/basic_host.go:142 +0x55
  gx/ipfs/QmaijwHnbD4SabGA8C2fN9gchptLvRe2RxqTU5XkjAGBw5/go-libp2p-swarm.(*Swarm).SetStreamHandler.func1()
      /Users/justin/go/src/gx/ipfs/QmaijwHnbD4SabGA8C2fN9gchptLvRe2RxqTU5XkjAGBw5/go-libp2p-swarm/swarm.go:269 +0x52
  gx/ipfs/Qma887khroMXGLJuHLYqqDZXHivAfFPxd2hQ8Z5kucMWTM/go-peerstream.(*Swarm).addConn.func1()
      /Users/justin/go/src/gx/ipfs/Qma887khroMXGLJuHLYqqDZXHivAfFPxd2hQ8Z5kucMWTM/go-peerstream/conn.go:213 +0x92
==================
magik6k commented 7 years ago

Duplicate of #3992

JustinDrake commented 7 years ago

Thanks. I'm actually using a small subset of go-ipfs, and this is the only race that is triggered for me (unlike when running the full daemon).

Stebalien commented 7 years ago

This is actually an intentional, benign race to avoid atomic operations on the fast path. I wonder if there's some way to tell go that.

Basically, this check can fail at most once per goroutine. If the check fails, we'll take the lock, perform the non-racy check, then record that the check has actually succeeded.

whyrusleeping commented 7 years ago

@Stebalien maybe we should just do an atomic operation to make go happier?

Stebalien commented 7 years ago

Probably. Conversation continued on this PR: https://github.com/multiformats/go-multistream/pull/17

Jorropo commented 1 year ago

This was fixed in https://github.com/multiformats/go-multistream/pull/21 (afait, anyway fixed)

This is actually an intentional, benign race to avoid atomic operations on the fast path. I wonder if there's some way to tell go that.

I know the value to answer to 2017 comment is extremely low but anyway. sync/atomic is the way to tell go that, on amd64 simple loads and writes compiles to plain old boring memory loads and writes and have no performance costs because amd64's whole memory model is consistent that way anyway.