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.

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

Bugfix: Websocket ping/pong improvements #406

Merged
merged 10 commits into from Jan 3, 2020

Conversation

@gloriousCode
Copy link
Collaborator

gloriousCode commented Dec 31, 2019

Alt Text

PR Description

This is a pretty basic PR which fixes up an issue where a ping message could theoretically crash the entire application due to a lack of locking. Turns out, only BTSE had the potential for this, but I've standardised ping handling a bit more while I was looking around. Checked each exchange websocket documentation to verify whether any ping pongs needed to be added or removed.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)

How has this been tested

  • go test ./... -race
  • golangci-lint run
  • TestSetupPingHandler()

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation and regenerated documentation via the documentation tool
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally and on Travis with my changes
  • Any dependent changes have been merged and published in downstream modules
… style or our own
@gloriousCode gloriousCode self-assigned this Dec 31, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 31, 2019

Codecov Report

Merging #406 into master will increase coverage by 0.02%.
The diff coverage is 25.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #406      +/-   ##
==========================================
+ Coverage   41.09%   41.11%   +0.02%     
==========================================
  Files         163      163              
  Lines       39042    39059      +17     
==========================================
+ Hits        16043    16061      +18     
+ Misses      21858    21850       -8     
- Partials     1141     1148       +7
Impacted Files Coverage Δ
exchanges/gateio/gateio_websocket.go 17.17% <ø> (ø) ⬆️
exchanges/btse/btse_websocket.go 0% <0%> (ø) ⬆️
exchanges/bitstamp/bitstamp_websocket.go 0% <0%> (ø) ⬆️
exchanges/poloniex/poloniex_websocket.go 12.21% <0%> (ø) ⬆️
exchanges/huobi/huobi_websocket.go 0% <0%> (ø) ⬆️
exchanges/coinut/coinut_websocket.go 0% <0%> (ø) ⬆️
exchanges/coinbene/coinbene_websocket.go 0% <0%> (ø) ⬆️
exchanges/btcmarkets/btcmarkets_websocket.go 0% <0%> (ø) ⬆️
exchanges/kraken/kraken_websocket.go 0% <0%> (ø) ⬆️
exchanges/bitfinex/bitfinex_websocket.go 0% <0%> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 98a277a...9cf9e3b. Read the comment docs.

Copy link
Collaborator

shazbert left a comment

Nice work!

}
k.WebsocketConn.SetupPingHandler(wshandler.WebsocketPingHandler{

This comment has been minimized.

Copy link
@shazbert

shazbert Jan 2, 2020

Collaborator

should set message type here

This comment has been minimized.

Copy link
@thrasher-

thrasher- Jan 3, 2020

Collaborator

Just reporting that the latest commit fixes the [ERROR] | 03/01/2020 11:23:24 | Kraken failed to send message to websocket {"event":"ping"} error

if handler.UseGorillaHandler {
h := func(msg string) error {
err := w.Connection.WriteControl(handler.MessageType, []byte(msg), time.Now().Add(handler.Delay))
if e, ok := err.(net.Error); ok && e.Temporary() {

This comment has been minimized.

Copy link
@shazbert

shazbert Jan 2, 2020

Collaborator

should also test for elseif err == websocket.ErrCloseSent and return nil

wc.SetupPingHandler(WebsocketPingHandler{
UseGorillaHandler: true,
MessageType: websocket.TextMessage,
Message: []byte(Ping),

This comment has been minimized.

Copy link
@shazbert

shazbert Jan 2, 2020

Collaborator

rm not being used when using gorilla handler

@@ -33,7 +33,10 @@ func (b *BTSE) WsConnect() error {
if err != nil {
return err
}
go b.Pinger()
b.WebsocketConn.SetupPingHandler(wshandler.WebsocketPingHandler{

This comment has been minimized.

Copy link
@shazbert

shazbert Jan 2, 2020

Collaborator

just making sure this doesn't need handler.Message

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Jan 2, 2020

Author Collaborator

Not for this one
From the docs:

To ensure that your session remains active, send a Ping frame before you reach the 60 second timeout.

Previous implementation was:
b.WebsocketConn.Connection.WriteMessage(websocket.PingMessage, nil)
which is the same as what's happening here

edit: I also haven't been disconnected while using BTSE in testing

…ds `if err == websocket.ErrCloseSent {` to ping func
@thrasher- thrasher- changed the title (bugfix) Websocket ping/pong improvements Bugfix: Websocket ping/pong improvements Jan 3, 2020
Copy link
Collaborator

thrasher- left a comment

Great stuff! Once again, only a MINOR nit and this is just for better easier display when sending websocket data. Have tested running this with all websocket exchanges enabled and have seen Binance using the gorilla ping/pong frame handling well

exchanges/websocket/wshandler/wshandler.go Outdated Show resolved Hide resolved
Copy link
Collaborator

shazbert left a comment

tACK!

Copy link
Collaborator

thrasher- left a comment

tACK! Great stuff 👍

@xtda
xtda approved these changes Jan 3, 2020
Copy link
Member

xtda left a comment

tested approved 🌮

Copy link
Collaborator

MadCozBadd left a comment

tACK!!

@thrasher- thrasher- merged commit 44ac358 into thrasher-corp:master Jan 3, 2020
3 of 5 checks passed
3 of 5 checks passed
CodeLingo timed out
Details
codecov/patch 25.92% of diff hit (target 41.09%)
Details
Travis CI - Pull Request Build Passed
Details
codecov/project 41.11% (+0.02%) compared to 98a277a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.