Race condition

[JustinDrake]

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]

Duplicate of #3992

[JustinDrake]

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]

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]

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

[Stebalien]

Probably. Conversation continued on this PR: multiformats/go-multistream#17

[Jorropo]

This was fixed in multiformats/go-multistream#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.