search

bitfinexcom / bitfinex-api-go

BITFINEX Go trading API - Bitcoin, Litecoin, and Ether exchange
https://www.bitfinex.com/
MIT License
303 stars 222 forks source link

Websocket logic has a data race #185

Closed andreygrehov closed 4 years ago

andreygrehov commented 4 years ago

Issue type

Brief description

Data race which is most likely caused by one concurrent reader/writer gorilla/websocket constraint. See https://godoc.org/github.com/gorilla/websocket#hdr-Concurrency for more details.

Steps to reproduce

go run -race https://raw.githubusercontent.com/bitfinexcom/bitfinex-api-go/master/examples/v2/ws-book/main.go

Additional Notes:
==================
WARNING: DATA RACE
Read at 0x00c0001521d8 by goroutine 21:
  github.com/gorilla/websocket.(*Conn).beginMessage()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:459 +0x5e
  github.com/gorilla/websocket.(*Conn).NextWriter()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:500 +0xae
  github.com/gorilla/websocket.(*Conn).WriteMessage()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:756 +0x261
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*ws).Send()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/transport.go:89 +0x389
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).EnableFlag()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/api.go:33 +0x1ae
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).checkResubscription()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:489 +0xaac
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).handleOpen()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:533 +0x169
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).handleEvent()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/events.go:119 +0x469
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).handleMessage()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:461 +0x10a
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).listenUpstream()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:421 +0x221

Previous write at 0x00c0001521d8 by main goroutine:
  github.com/gorilla/websocket.(*Conn).NextWriter()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:503 +0xdd
  github.com/gorilla/websocket.(*Conn).WriteMessage()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:756 +0x261
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*ws).Send()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/transport.go:89 +0x389
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).subscribeBySocket()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/api.go:58 +0xed
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).Subscribe()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/api.go:79 +0x102
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).SubscribeTrades()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/api.go:101 +0x1e8
  main.main()
      /Users/andreygrehov/bfxdatarace/main.go:34 +0x34d

Goroutine 21 (running) created at:
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).connectSocket()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:395 +0x32e
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).Connect()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:220 +0xbf
  main.main()
      /Users/andreygrehov/bfxdatarace/main.go:18 +0xe6
==================
==================
WARNING: DATA RACE
Read at 0x00c0001521e8 by goroutine 21:
  github.com/gorilla/websocket.(*messageWriter).flushFrame()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:596 +0x6e2
  github.com/gorilla/websocket.(*messageWriter).Close()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:711 +0x72
  github.com/gorilla/websocket.(*Conn).WriteMessage()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:763 +0x315
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*ws).Send()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/transport.go:89 +0x389
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).EnableFlag()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/api.go:33 +0x1ae
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).checkResubscription()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:489 +0xaac
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).handleOpen()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:533 +0x169
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).handleEvent()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/events.go:119 +0x469
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).handleMessage()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:461 +0x10a
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).listenUpstream()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:421 +0x221

Previous write at 0x00c0001521e8 by main goroutine:
  github.com/gorilla/websocket.(*messageWriter).flushFrame()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:606 +0x8cc
  github.com/gorilla/websocket.(*messageWriter).Close()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:711 +0x72
  github.com/gorilla/websocket.(*Conn).WriteMessage()
      /Users/andreygrehov/go/src/github.com/gorilla/websocket/conn.go:763 +0x315
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*ws).Send()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/transport.go:89 +0x389
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).subscribeBySocket()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/api.go:58 +0xed
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).Subscribe()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/api.go:79 +0x102
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).SubscribeTrades()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/api.go:101 +0x1e8
  main.main()
      /Users/andreygrehov/bfxdatarace/main.go:34 +0x34d

Goroutine 21 (running) created at:
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).connectSocket()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:395 +0x32e
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).Connect()
      /Users/andreygrehov/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:220 +0xbf
  main.main()
      /Users/andreygrehov/bfxdatarace/main.go:18 +0xe6
==================
JacobPlaster commented 4 years ago

Ahh thanks @andreygrehov, seems like this was introduced with the num milit-ws connection multiplexer. Ill look into it now

JacobPlaster commented 4 years ago

Fixed in https://github.com/bitfinexcom/bitfinex-api-go/pull/186. The solution was to use a channel/writer pattern to ensure that websocket send messages are all processed synchronously on a single routine/thread.

andreygrehov commented 4 years ago

@JacobPlaster thanks for a quick turnaround. Code reviewed the PR.

JacobPlaster commented 4 years ago

Merged in #186, this can be closed now.

andreygrehov commented 4 years ago

Teamwork. Appreciate it @JacobPlaster.