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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature+Bugfix: Engine websocket management #360

Merged

Conversation

@gloriousCode
Copy link
Collaborator

commented Sep 24, 2019

image

Description

Many of the websocket improvements I've been making have been for master branch. The engine branch introduces features like syncer.go which alleviates a lot of the self-managed handling of the websocket traffic and connection statuses. This PR removes a lot of the self-management, relies on syncer.go to manage when to use websocket for data and simplifies the connection manager to only care about the connection.

One of the biggest bugs I introduced was the shutdown channel closure timeout. If go could not close the websocket.ShutdownC channel in 15 seconds, it would give up. The following test demonstrates that even if a channel is busy and a close is called, it will eventually close successfully.

https://play.golang.org/p/RYiouqq2Twk

Another issue was that if the websocket was disconnected and failed to reconnect, it would eventually reach a situation where it would just output logs saying that it can't reconnect anymore. The new connection manager will try to reconnect forever unless the websocket is disabled.

More minor things have been cleaned up like the traffic monitor. It doesn't use channels to manage status and doesn't have a flow-on timeout tier of disconnections. By default, if the websocket hasn't received data for 2 minutes, it will call a shutdown, which the connection monitor will reconnect and the subscription handler will resubscribe to everything.

I've also re-added all the websocket connection tests that were removed in #328 which none of us noticed 馃槅

Type of change

Please delete options that are not relevant and add an x in [] as item is complete.

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

How Has This Been Tested?

wshandler_test has fancy new tests. Running the application, killing the connection, enabling connection and watching all exchanges reconnect to the websocket and resubscribe
Also the usual go test ./... -racerino

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
@gloriousCode gloriousCode self-assigned this Sep 24, 2019
Copy link
Collaborator

left a comment

Awesome work!

exchanges/websocket/wshandler/wshandler.go Outdated Show resolved Hide resolved
exchanges/websocket/wsorderbook/wsorderbook_test.go Outdated Show resolved Hide resolved
engine/routines.go Outdated Show resolved Hide resolved
@shazbert

This comment has been minimized.

Copy link
Collaborator

commented Sep 25, 2019

image
tACK!
Great update!

@thrasher-

This comment has been minimized.

Copy link
Collaborator

commented Sep 26, 2019

Please rebase

@gloriousCode gloriousCode force-pushed the gloriousCode:syncer-socket-satisfaction branch from d9ee8b7 to 27ae443 Sep 26, 2019
Copy link
Collaborator

left a comment

Great work @gloriousCode. Some really nice improvements here, will do a thorough test on my next passthrough.

config/config.go Outdated Show resolved Hide resolved
config/config.go Show resolved Hide resolved
exchanges/websocket/wshandler/wshandler_test.go Outdated Show resolved Hide resolved
exchanges/websocket/wshandler/wshandler_test.go Outdated Show resolved Hide resolved
exchanges/websocket/wshandler/wshandler_types.go Outdated Show resolved Hide resolved
Copy link

left a comment

LGTM

@codecov

This comment has been minimized.

Copy link

commented Sep 27, 2019

Codecov Report

Merging #360 into engine will increase coverage by 0.57%.
The diff coverage is 76.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##           engine     #360      +/-   ##
==========================================
+ Coverage   41.23%   41.81%   +0.57%     
==========================================
  Files         149      149              
  Lines       33544    33598      +54     
==========================================
+ Hits        13831    14048     +217     
+ Misses      18780    18595     -185     
- Partials      933      955      +22
Impacted Files Coverage 螖
engine/exchange.go 29.37% <酶> (酶) 猬嗭笍
config/config_types.go 79.41% <酶> (酶) 猬嗭笍
exchanges/poloniex/poloniex_websocket.go 15.19% <0%> (酶) 猬嗭笍
exchanges/exchange.go 64.4% <0%> (酶) 猬嗭笍
exchanges/kraken/kraken_websocket.go 0% <0%> (酶) 猬嗭笍
exchanges/hitbtc/hitbtc_websocket.go 0% <0%> (酶) 猬嗭笍
exchanges/coinbasepro/coinbasepro_websocket.go 0% <0%> (酶) 猬嗭笍
engine/routines.go 0% <0%> (酶) 猬嗭笍
exchanges/zb/zb_websocket.go 1.86% <0%> (酶) 猬嗭笍
exchanges/bitfinex/bitfinex_websocket.go 0% <0%> (酶) 猬嗭笍
... and 30 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 6951b4c...7acb04d. Read the comment docs.

Copy link
Collaborator

left a comment

Websocket traffic timeout might need to be extended or re-thought.

exchanges/websocket/wshandler/wshandler.go Outdated Show resolved Hide resolved
exchanges/websocket/wshandler/wshandler.go Outdated Show resolved Hide resolved
// trafficMonitor monitors traffic and switches connection modes for websocket
// trafficMonitor uses a timer of WebsocketTrafficLimitTime and once it expires
// Will reconnect if the TrafficAlert channel has not received any data
// The trafficTimer will reset on each traffic alert
func (w *Websocket) trafficMonitor(wg *sync.WaitGroup) {

This comment has been minimized.

Copy link
@shazbert

shazbert Oct 1, 2019

Collaborator

Two routines are being spawned in production for the same exchange.
image

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Oct 1, 2019

Author Collaborator

I've added some bools to prevent multiple traffic monitors running. I've also reduced the amount of Connect() being called to prevent the situation arising

for {
select {
case <-w.ShutdownC: // Returns on shutdown channel close
case <-w.ShutdownC:
if w.verbose {

This comment has been minimized.

Copy link
@shazbert

shazbert Oct 1, 2019

Collaborator

verbose does not like being set from the command line on this function.

This comment has been minimized.

Copy link
@gloriousCode

gloriousCode Oct 1, 2019

Author Collaborator

I've set websocket verbosity to marry up to exchange verbosity.
gct -verbose is separate to gct -exchangeverbose. Running it with -exchangeverbose works for me.

gloriousCode added 17 commits Sep 18, 2019
鈥urpose is to remove the traffic monitoring and dropping as syncer.go is a better manager
鈥locks and prevent races
鈥eletes WebsocketReset. Utilises ReadMessageErrors channel for all websocket readmessages to analyse when an error returned is due to a disconnect
鈥scribes after disconnection
鈥ic monitor should die. Default to two minutes of no traffic activity. Increases test coverage and updates existing tests to work with new technologic. RE-ADDS TESTS I ACCIDENTALLY DELETED FROM PREVIOUS PR
鈥onnections. Increases test coverage. Re-adds tests that were ACCIDENTALLY DELETED. Removes unused websocket channels. Bug fix for traffic monitor to shutdown via goroutine instead of killing itself
鈥y to syncer for when websocket is switched to rest and then back, you get a log notifying of the return. Fixes okgroup bug where ws message is sent on a disconnected ws, causing panic. Renames setConnectionStatus to setConnectedStatus. Puts connection monitor log behind verbose bool
鈥ou liked it or not. Removes demonstration test
鈥 timers properly
鈥. Handles unhandled errors
鈥onnected() to prevent a shutdown from recurring
鈥 Expands disconnection error definition. Removes routine disconnection error handling. Ensures only one traffic monitor can ever be run. Renames subscriptionLock to subscriptionMutext for consistency
@gloriousCode gloriousCode force-pushed the gloriousCode:syncer-socket-satisfaction branch from e318b61 to edce59a Oct 1, 2019
gloriousCode added 3 commits Oct 1, 2019
鈥on-popular currencies
Copy link

left a comment

LGTM

Copy link
Collaborator

left a comment

Changes look good, reconnect works.

Copy link
Collaborator

left a comment

tACK via ./gocryptotrader.exe --config=config_example.json --exchangeverbose=true --exchangewebsocketsupport=true --syncmanager=false, killed all connections and watched them respawn accordingly. Also disconnected network adapter and watched them all come back online. Very cool stuff 馃憤

@xtda
xtda approved these changes Oct 1, 2019
Copy link
Member

left a comment

Tested and working great here

Only a couple of minor things

going on from what thrasher mentioned is using SetReadDeadline and ping/pong handlers, to monitor websocket disconnects/reconnection/handle rest switch over

But as trafficMonitor already does this it might not be useful

@thrasher- thrasher- merged commit c2a3330 into thrasher-corp:engine Oct 1, 2019
4 of 5 checks passed
4 of 5 checks passed
Review Action 0 issues found before hitting an error.
Details
Travis CI - Pull Request Build Passed
Details
codecov/patch 76.69% of diff hit (target 41.23%)
Details
codecov/project 41.81% (+0.57%) compared to 6951b4c
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@gloriousCode gloriousCode referenced this pull request Oct 4, 2019
10 of 10 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can鈥檛 perform that action at this time.