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.

Sign up for GitHub

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

Jump to bottom

Small race fixes #186

Merged
merged 6 commits into from
Jul 30, 2019
Merged

Small race fixes #186

merged 6 commits into from
Jul 30, 2019

Conversation

JacobPlaster
Copy link
Contributor

@JacobPlaster JacobPlaster commented Jul 29, 2019 •
edited
Loading

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:

  • Changed rest.Trades to use the correct trade type for authenticated trade data. Not really a breaking change as this was not working before but the data type has changed.

New features:

None

Fixes:

  • Multiple writes when trying to send a websocket message race
  • Building snapshot and get orderbook race

PR status:

  • Version bumped
  • Change-log updated

@JacobPlaster JacobPlaster mentioned this pull request Jul 29, 2019
Closed
4 tasks
andreygrehov
andreygrehov reviewed Jul 29, 2019
View reviewed changes
v2/websocket/transport.go Outdated
case <- w.quit: // ws closed
return
case message := <- w.writeChan:
err := w.ws.WriteMessage(websocket.TextMessage, message)
Copy link
Contributor

@andreygrehov andreygrehov Jul 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error handling is missed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oppps, good spot. This line needs to removed completely as ws.Write is used instead a few lines down

andreygrehov
andreygrehov reviewed Jul 29, 2019
View reviewed changes
v2/websocket/transport.go Outdated
@@ -85,8 +90,9 @@ func (w *ws) Send(ctx context.Context, msg interface{}) error {
return fmt.Errorf("websocket connection closed")
default:
}
w.log.Debug("ws->srv: %s", string(bs))
err = w.ws.WriteMessage(websocket.TextMessage, bs)
w.log.Info("ws->srv: %s", string(bs))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it make sense to keep it under debug level?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep good point, changing back to debug

@andreygrehov
Copy link
Contributor

Seems like the change affected the v2 integration tests.

@JacobPlaster
Copy link
Contributor Author

JacobPlaster commented Jul 29, 2019 •
edited
Loading

Seems like the change affected the v2 integration tests.

Checking now

@andreygrehov
Copy link
Contributor

@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
Copy link
Contributor Author

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
Copy link
Contributor

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

@andreygrehov
Copy link
Contributor

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

@JacobPlaster
Copy link
Contributor Author

No problem, thanks again

@JacobPlaster JacobPlaster changed the title [wip] Small race fixes Small race fixes Jul 30, 2019
@prdn prdn merged commit 5e80cb1 into bitfinexcom:master Jul 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andreygrehov andreygrehov andreygrehov approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@JacobPlaster @andreygrehov @prdn