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

Small race fixes #186

Closed JacobPlaster closed 4 years ago

JacobPlaster commented 4 years ago

Description:

There are a few race conditions which seemed to have snook in with the introduction to the mulit-websocket connection multiplexer. This pull request addresses those race conditions which were found using go run -race.

Breaking changes:

New features:

None

Fixes:

PR status:

andreygrehov commented 4 years ago

Seems like the change affected the v2 integration tests.

JacobPlaster commented 4 years ago

Seems like the change affected the v2 integration tests.

Checking now

andreygrehov commented 4 years ago

@JacobPlaster thank you. The code looks good to me. I ran a few tests and found a corner case. Enabling order book checksum verification leads to another data race. Pasting more details:

WARNING: DATA RACE
Read at 0x00c0001881d8 by goroutine 25:
  github.com/gorilla/websocket.(*Conn).beginMessage()
      /Users/andrey/Code/go/src/github.com/gorilla/websocket/conn.go:459 +0x5e
  github.com/gorilla/websocket.(*Conn).NextWriter()
      /Users/andrey/Code/go/src/github.com/gorilla/websocket/conn.go:500 +0xae
  github.com/gorilla/websocket.(*Conn).WriteMessage()
      /Users/andrey/Code/go/src/github.com/gorilla/websocket/conn.go:753 +0x261
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*ws).Send()
      /Users/andrey/Code/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/andrey/Code/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/andrey/Code/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/andrey/Code/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/andrey/Code/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/andrey/Code/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/andrey/Code/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:421 +0x221

Previous write at 0x00c0001881d8 by main goroutine:
  github.com/gorilla/websocket.(*messageWriter).endMessage()
      /Users/andrey/Code/go/src/github.com/gorilla/websocket/conn.go:526 +0xb9
  github.com/gorilla/websocket.(*messageWriter).flushFrame()
      /Users/andrey/Code/go/src/github.com/gorilla/websocket/conn.go:613 +0x92e
  github.com/gorilla/websocket.(*messageWriter).Close()
      /Users/andrey/Code/go/src/github.com/gorilla/websocket/conn.go:711 +0xba
  github.com/gorilla/websocket.(*Conn).WriteMessage()
      /Users/andrey/Code/go/src/github.com/gorilla/websocket/conn.go:760 +0x315
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*ws).Send()
      /Users/andrey/Code/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/andrey/Code/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/andrey/Code/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/api.go:79 +0x102
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).SubscribeBook()
      /Users/andrey/Code/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/api.go:121 +0x454
  main.main()
      /Users/andrey/bitfinex-api-go/examples/v2/ws-book/main.go:26 +0x246

Goroutine 25 (running) created at:
  github.com/bitfinexcom/bitfinex-api-go/v2/websocket.(*Client).connectSocket()
      /Users/andrey/Code/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/andrey/Code/go/src/github.com/bitfinexcom/bitfinex-api-go/v2/websocket/client.go:220 +0xbf
  main.main()
      /Users/andrey/bitfinex-api-go/examples/v2/ws-book/main.go:18 +0xe6
JacobPlaster commented 4 years ago

Thanks @andreygrehov, the help is appreciated! The above error was found without using this PR fix right? I've tested this version out quite a lot and cant seem to reproduce any race conditions

andreygrehov commented 4 years ago

@JacobPlaster I tested your branch. Let me double-check.

andreygrehov commented 4 years ago

@JacobPlaster apologies. I indeed mistested it. No more races.

JacobPlaster commented 4 years ago

No problem, thanks again