Feature+Bugfix: Engine websocket management (thrasher-corp#360)

* Initial commit tearing down the websocket connection management. The purpose is to remove the traffic monitoring and dropping as syncer.go is a better manager

* Adds a readwrite mutex and helper functions to minimise inline lock/unlocks and prevent races

* Creates new WebsocketType struct to contain all parameters required. Deletes WebsocketReset. Utilises ReadMessageErrors channel for all websocket readmessages to analyse when an error returned is due to a disconnect

* Fixes issue with syncer trying to connect while connecting

* Simplifies initialisation function for websocket. Reconnects and resubscribes after disconnection

* Adds WebsocketTimeout config value to dictate when the websocket traffic 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

* Removes snapshot override as its always necessary when considering reconnections. 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

* Fixes gateio bug for authentication errors when null. Adds little entry 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

* Fixes lingering races. Fixes bug where websocket was enabled whether you liked it or not. Removes demonstration test

* Fixes log message, renames unc, removes comments

* Fixes data race

* Removes verbosity, ensures shutdown sets connection status appropriately

* Removes go routine causing CPU spike. Stops timers properly and resets timers properly

* Renames `WsEnabled` to `Enabled`. Increases test coverage. Fixes typos. Handles unhandled errors

* The forgotten lint

* With using RWlocks, removes the channel nil check and relies on !w.IsConnected() to prevent a shutdown from recurring

* Removes extra closure step in the defer as it causes all the issues

* Prevents timer channel hangups. Minimises use of websocket Connect(). Expands disconnection error definition. Removes routine disconnection error handling. Ensures only one traffic monitor can ever be run. Renames subscriptionLock to subscriptionMutext for consistency

* Extends timeout to 30 seconds to cover for non-popular exchanges and non-popular currencies

* Updates test from rebase to use new websocket setup function

* Fixes test to ensure it tests what it says it does

config/config.go

@@ -41,6 +41,7 @@ const (
 	configDefaultWebsocketResponseCheckTimeout = time.Millisecond * 30
 	configDefaultWebsocketResponseMaxLimit     = time.Second * 7
 	configDefaultWebsocketOrderbookBufferLimit = 5
+	configDefaultWebsocketTrafficTimeout       = time.Second * 30
 	configMaxAuthFailures                      = 3
 	defaultNTPAllowedDifference                = 50000000
 	defaultNTPAllowedNegativeDifference        = 50000000
@@ -1024,6 +1025,11 @@ func (c *Config) CheckExchangeConfigValues() error {
 					c.Exchanges[i].Name, configDefaultWebsocketResponseMaxLimit)
 				c.Exchanges[i].WebsocketResponseMaxLimit = configDefaultWebsocketResponseMaxLimit
 			}
+			if c.Exchanges[i].WebsocketTrafficTimeout <= 0 {
+				log.Warnf(log.ExchangeSys, "Exchange %s Websocket response traffic timeout value not set, defaulting to %v.",
+					c.Exchanges[i].Name, configDefaultWebsocketTrafficTimeout)
+				c.Exchanges[i].WebsocketTrafficTimeout = configDefaultWebsocketTrafficTimeout
+			}
 			if c.Exchanges[i].WebsocketOrderbookBufferLimit <= 0 {
 				log.Warnf(log.ExchangeSys, "Exchange %s Websocket orderbook buffer limit value not set, defaulting to %v.",
 					c.Exchanges[i].Name, configDefaultWebsocketOrderbookBufferLimit)

config/config_test.go

@@ -1451,6 +1451,7 @@ func TestCheckExchangeConfigValues(t *testing.T) {
 	cfg.Exchanges[0].WebsocketResponseMaxLimit = 0
 	cfg.Exchanges[0].WebsocketResponseCheckTimeout = 0
 	cfg.Exchanges[0].WebsocketOrderbookBufferLimit = 0
+	cfg.Exchanges[0].WebsocketTrafficTimeout = 0
 	cfg.Exchanges[0].HTTPTimeout = 0
 	err = cfg.CheckExchangeConfigValues()
 	if err != nil {
@@ -1465,6 +1466,10 @@ func TestCheckExchangeConfigValues(t *testing.T) {
 		t.Errorf("expected exchange %s to have updated WebsocketOrderbookBufferLimit value",
 			cfg.Exchanges[0].Name)
 	}
+	if cfg.Exchanges[0].WebsocketTrafficTimeout == 0 {
+		t.Errorf("expected exchange %s to have updated WebsocketTrafficTimeout value",
+			cfg.Exchanges[0].Name)
+	}
 	if cfg.Exchanges[0].HTTPTimeout == 0 {
 		t.Errorf("expected exchange %s to have updated HTTPTimeout value",
 			cfg.Exchanges[0].Name)

config/config_types.go

@@ -59,6 +59,7 @@ type ExchangeConfig struct {
 	HTTPRateLimiter               *HTTPRateLimitConfig   `json:"httpRateLimiter,omitempty"`
 	WebsocketResponseCheckTimeout time.Duration          `json:"websocketResponseCheckTimeout"`
 	WebsocketResponseMaxLimit     time.Duration          `json:"websocketResponseMaxLimit"`
+	WebsocketTrafficTimeout       time.Duration          `json:"websocketTrafficTimeout"`
 	WebsocketOrderbookBufferLimit int                    `json:"websocketOrderbookBufferLimit"`
 	ProxyAddress                  string                 `json:"proxyAddress,omitempty"`
 	BaseCurrencies                currency.Currencies    `json:"baseCurrencies"`

engine/exchange.go

@@ -228,7 +228,6 @@ func LoadExchange(name string, useWG bool, wg *sync.WaitGroup) error {
 			if exchCfg.Features.Supports.RESTCapabilities.AutoPairUpdates {
 				exchCfg.Features.Enabled.AutoPairUpdates = false
 			}
-
 		}
 	}
 

engine/routines.go

@@ -354,39 +354,12 @@ func Websocketshutdown(ws *wshandler.Websocket) error {
 	}
 }
 
-// streamDiversion is a diversion switch from websocket to REST or other
-// alternative feed
-func streamDiversion(ws *wshandler.Websocket) {
-	wg.Add(1)
-	defer wg.Done()
-
-	for {
-		select {
-		case <-shutdowner:
-			return
-
-		case <-ws.Connected:
-			if Bot.Settings.Verbose {
-				log.Debugf(log.WebsocketMgr, "exchange %s websocket feed connected\n", ws.GetName())
-			}
-
-		case <-ws.Disconnected:
-			if Bot.Settings.Verbose {
-				log.Debugf(log.WebsocketMgr, "exchange %s websocket feed disconnected, switching to REST functionality\n",
-					ws.GetName())
-			}
-		}
-	}
-}
-
 // WebsocketDataHandler handles websocket data coming from a websocket feed
 // associated with an exchange
 func WebsocketDataHandler(ws *wshandler.Websocket) {
 	wg.Add(1)
 	defer wg.Done()
 
-	go streamDiversion(ws)
-
 	for {
 		select {
 		case <-shutdowner:
@@ -407,14 +380,7 @@ func WebsocketDataHandler(ws *wshandler.Websocket) {
 				}
 
 			case error:
-				switch {
-				case strings.Contains(d.Error(), "close 1006"):
-					go ws.WebsocketReset()
-					continue
-				default:
-					log.Errorf(log.WebsocketMgr, "routines.go exchange %s websocket error - %s", ws.GetName(), data)
-				}
-
+				log.Errorf(log.WebsocketMgr, "routines.go exchange %s websocket error - %s", ws.GetName(), data)
 			case wshandler.TradeData:
 				// Trade Data
 				// if Bot.Settings.Verbose {

engine/syncer.go

@@ -286,7 +286,7 @@ func (e *ExchangeCurrencyPairSyncer) worker() {
 			supportsRESTTickerBatching := Bot.Exchanges[x].SupportsRESTTickerBatchUpdates()
 			var usingREST bool
 			var usingWebsocket bool
-
+			var switchedToRest bool
 			if Bot.Exchanges[x].SupportsWebsocket() && Bot.Exchanges[x].IsWebsocketEnabled() {
 				ws, err := Bot.Exchanges[x].GetWebsocket()
 				if err != nil {
@@ -346,7 +346,12 @@ func (e *ExchangeCurrencyPairSyncer) worker() {
 						log.Errorf(log.SyncMgr, "failed to get item. Err: %s\n", err)
 						continue
 					}
-
+					if switchedToRest && usingWebsocket {
+						log.Infof(log.SyncMgr,
+							"%s %s: Websocket re-enabled, switching from rest to websocket\n",
+							c.Exchange, FormatCurrency(p).String())
+						switchedToRest = false
+					}
 					if e.Cfg.SyncTicker {
 						if !e.isProcessing(exchangeName, c.Pair, c.AssetType, SyncItemTicker) {
 							if c.Ticker.LastUpdated.IsZero() || time.Since(c.Ticker.LastUpdated) > defaultSyncerTimeout {
@@ -362,6 +367,7 @@ func (e *ExchangeCurrencyPairSyncer) worker() {
 										log.Warnf(log.SyncMgr,
 											"%s %s: No ticker update after 10 seconds, switching from websocket to rest\n",
 											c.Exchange, FormatCurrency(p).String())
+										switchedToRest = true
 										e.setProcessing(c.Exchange, c.Pair, c.AssetType, SyncItemTicker, false)
 									}
 								}
@@ -425,6 +431,7 @@ func (e *ExchangeCurrencyPairSyncer) worker() {
 										log.Warnf(log.SyncMgr,
 											"%s %s: No orderbook update after 15 seconds, switching from websocket to rest\n",
 											c.Exchange, FormatCurrency(c.Pair).String())
+										switchedToRest = true
 										e.setProcessing(c.Exchange, c.Pair, c.AssetType, SyncItemOrderbook, false)
 									}
 								}
@@ -491,7 +498,7 @@ func (e *ExchangeCurrencyPairSyncer) Start() {
 				usingREST = true
 			}
 
-			if !ws.IsConnected() {
+			if !ws.IsConnected() && !ws.IsConnecting() {
 				go WebsocketDataHandler(ws)
 
 				err = ws.Connect()

exchanges/alphapoint/alphapoint_websocket.go

@@ -38,6 +38,7 @@ func (a *Alphapoint) WebsocketClient() {
 		for a.Enabled {
 			msgType, resp, err := a.WebsocketConn.ReadMessage()
 			if err != nil {
+				a.Websocket.ReadMessageErrors <- err
 				log.Error(log.ExchangeSys, err)
 				break
 			}

exchanges/binance/binance_websocket.go

@@ -21,8 +21,8 @@ const (
 	binanceDefaultWebsocketURL = "wss://stream.binance.com:9443"
 )
 
-// WSConnect intiates a websocket connection
-func (b *Binance) WSConnect() error {
+// WsConnect intiates a websocket connection
+func (b *Binance) WsConnect() error {
 	if !b.Websocket.IsEnabled() || !b.IsEnabled() {
 		return errors.New(wshandler.WebsocketNotEnabled)
 	}
@@ -87,7 +87,7 @@ func (b *Binance) WsHandleData() {
 		default:
 			read, err := b.WebsocketConn.ReadMessage()
 			if err != nil {
-				b.Websocket.DataHandler <- err
+				b.Websocket.ReadMessageErrors <- err
 				return
 			}
 			b.Websocket.TrafficAlert <- struct{}{}
@@ -248,7 +248,7 @@ func (b *Binance) SeedLocalCache(p currency.Pair) error {
 	newOrderBook.Pair = p
 	newOrderBook.AssetType = asset.Spot
 
-	return b.Websocket.Orderbook.LoadSnapshot(&newOrderBook, false)
+	return b.Websocket.Orderbook.LoadSnapshot(&newOrderBook)
 }
 
 // UpdateLocalCache updates and returns the most recent iteration of the orderbook

exchanges/binance/binance_wrapper.go

@@ -113,15 +113,18 @@ func (b *Binance) Setup(exch *config.ExchangeConfig) error {
 		return err
 	}
 
-	err = b.Websocket.Setup(b.WSConnect,
-		nil,
-		nil,
-		exch.Name,
-		exch.Features.Enabled.Websocket,
-		exch.Verbose,
-		binanceDefaultWebsocketURL,
-		exch.API.Endpoints.WebsocketURL,
-		exch.API.AuthenticatedWebsocketSupport)
+	err = b.Websocket.Setup(
+		&wshandler.WebsocketSetup{
+			Enabled:                          exch.Features.Enabled.Websocket,
+			Verbose:                          exch.Verbose,
+			AuthenticatedWebsocketAPISupport: exch.API.AuthenticatedWebsocketSupport,
+			WebsocketTimeout:                 exch.WebsocketTrafficTimeout,
+			DefaultURL:                       binanceDefaultWebsocketURL,
+			ExchangeName:                     exch.Name,
+			RunningURL:                       exch.API.Endpoints.WebsocketURL,
+			Connector:                        b.WsConnect,
+		})
+
 	if err != nil {
 		return err
 	}

exchanges/bitfinex/bitfinex_websocket.go

@@ -133,6 +133,7 @@ func (b *Bitfinex) WsConnect() error {
 
 	resp, err := b.WebsocketConn.ReadMessage()
 	if err != nil {
+		b.Websocket.ReadMessageErrors <- err
 		return fmt.Errorf("%v unable to read from Websocket. Error: %s", b.Name, err)
 	}
 	b.Websocket.TrafficAlert <- struct{}{}
@@ -177,7 +178,7 @@ func (b *Bitfinex) WsDataHandler() {
 		default:
 			stream, err := b.WebsocketConn.ReadMessage()
 			if err != nil {
-				b.Websocket.DataHandler <- err
+				b.Websocket.ReadMessageErrors <- err
 				return
 			}
 			b.Websocket.TrafficAlert <- struct{}{}
@@ -481,7 +482,7 @@ func (b *Bitfinex) WsInsertSnapshot(p currency.Pair, assetType asset.Item, books
 	newOrderBook.AssetType = assetType
 	newOrderBook.Bids = bid
 	newOrderBook.Pair = p
-	err := b.Websocket.Orderbook.LoadSnapshot(&newOrderBook, false)
+	err := b.Websocket.Orderbook.LoadSnapshot(&newOrderBook)
 	if err != nil {
 		return fmt.Errorf("bitfinex.go error - %s", err)
 	}

exchanges/bitfinex/bitfinex_wrapper.go

@@ -114,15 +114,19 @@ func (b *Bitfinex) Setup(exch *config.ExchangeConfig) error {
 		return err
 	}
 
-	err = b.Websocket.Setup(b.WsConnect,
-		b.Subscribe,
-		b.Unsubscribe,
-		exch.Name,
-		exch.Features.Enabled.Websocket,
-		exch.Verbose,
-		bitfinexWebsocket,
-		exch.API.Endpoints.WebsocketURL,
-		exch.API.AuthenticatedWebsocketSupport)
+	err = b.Websocket.Setup(
+		&wshandler.WebsocketSetup{
+			Enabled:                          exch.Features.Enabled.Websocket,
+			Verbose:                          exch.Verbose,
+			AuthenticatedWebsocketAPISupport: exch.API.AuthenticatedWebsocketSupport,
+			WebsocketTimeout:                 exch.WebsocketTrafficTimeout,
+			DefaultURL:                       bitfinexWebsocket,
+			ExchangeName:                     exch.Name,
+			RunningURL:                       exch.API.Endpoints.WebsocketURL,
+			Connector:                        b.WsConnect,
+			Subscriber:                       b.Subscribe,
+			UnSubscriber:                     b.Unsubscribe,
+		})
 	if err != nil {
 		return err
 	}

exchanges/bitmex/bitmex_websocket.go

@@ -66,8 +66,8 @@ var (
 	pongChan = make(chan int, 1)
 )
 
-// WsConnector initiates a new websocket connection
-func (b *Bitmex) WsConnector() error {
+// WsConnect initiates a new websocket connection
+func (b *Bitmex) WsConnect() error {
 	if !b.Websocket.IsEnabled() || !b.IsEnabled() {
 		return errors.New(wshandler.WebsocketNotEnabled)
 	}
@@ -79,6 +79,7 @@ func (b *Bitmex) WsConnector() error {
 
 	p, err := b.WebsocketConn.ReadMessage()
 	if err != nil {
+		b.Websocket.ReadMessageErrors <- err
 		return err
 	}
 	b.Websocket.TrafficAlert <- struct{}{}
@@ -360,7 +361,7 @@ func (b *Bitmex) processOrderbook(data []OrderBookL2, action string, currencyPai
 		newOrderBook.Bids = bids
 		newOrderBook.AssetType = assetType
 		newOrderBook.Pair = currencyPair
-		err := b.Websocket.Orderbook.LoadSnapshot(&newOrderBook, false)
+		err := b.Websocket.Orderbook.LoadSnapshot(&newOrderBook)
 		if err != nil {
 			return fmt.Errorf("bitmex_websocket.go process orderbook error -  %s",
 				err)

exchanges/bitmex/bitmex_wrapper.go

@@ -137,15 +137,19 @@ func (b *Bitmex) Setup(exch *config.ExchangeConfig) error {
 		return err
 	}
 
-	err = b.Websocket.Setup(b.WsConnector,
-		b.Subscribe,
-		b.Unsubscribe,
-		exch.Name,
-		exch.Features.Enabled.Websocket,
-		exch.Verbose,
-		bitmexWSURL,
-		exch.API.Endpoints.WebsocketURL,
-		exch.API.AuthenticatedWebsocketSupport)
+	err = b.Websocket.Setup(
+		&wshandler.WebsocketSetup{
+			Enabled:                          exch.Features.Enabled.Websocket,
+			Verbose:                          exch.Verbose,
+			AuthenticatedWebsocketAPISupport: exch.API.AuthenticatedWebsocketSupport,
+			WebsocketTimeout:                 exch.WebsocketTrafficTimeout,
+			DefaultURL:                       bitmexWSURL,
+			ExchangeName:                     exch.Name,
+			RunningURL:                       exch.API.Endpoints.WebsocketURL,
+			Connector:                        b.WsConnect,
+			Subscriber:                       b.Subscribe,
+			UnSubscriber:                     b.Unsubscribe,
+		})
 	if err != nil {
 		return err
 	}

exchanges/bitstamp/bitstamp_websocket.go

@@ -62,7 +62,7 @@ func (b *Bitstamp) WsHandleData() {
 		default:
 			resp, err := b.WebsocketConn.ReadMessage()
 			if err != nil {
-				b.Websocket.DataHandler <- err
+				b.Websocket.ReadMessageErrors <- err
 				return
 			}
 			b.Websocket.TrafficAlert <- struct{}{}
@@ -78,7 +78,7 @@ func (b *Bitstamp) WsHandleData() {
 				if b.Verbose {
 					log.Debugf(log.ExchangeSys, "%v - Websocket reconnection request received", b.GetName())
 				}
-				go b.Websocket.WebsocketReset()
+				go b.Websocket.Shutdown() // Connection monitor will reconnect
 
 			case "data":
 				wsOrderBookTemp := websocketOrderBookResponse{}
@@ -248,7 +248,7 @@ func (b *Bitstamp) seedOrderBook() error {
 		newOrderBook.Pair = p[x]
 		newOrderBook.AssetType = asset.Spot
 
-		err = b.Websocket.Orderbook.LoadSnapshot(&newOrderBook, false)
+		err = b.Websocket.Orderbook.LoadSnapshot(&newOrderBook)
 		if err != nil {
 			return err
 		}

exchanges/bitstamp/bitstamp_wrapper.go

@@ -110,15 +110,19 @@ func (b *Bitstamp) Setup(exch *config.ExchangeConfig) error {
 		return err
 	}
 
-	err = b.Websocket.Setup(b.WsConnect,
-		b.Subscribe,
-		b.Unsubscribe,
-		exch.Name,
-		exch.Features.Enabled.Websocket,
-		exch.Verbose,
-		bitstampWSURL,
-		exch.API.Endpoints.WebsocketURL,
-		exch.API.AuthenticatedWebsocketSupport)
+	err = b.Websocket.Setup(
+		&wshandler.WebsocketSetup{
+			Enabled:                          exch.Features.Enabled.Websocket,
+			Verbose:                          exch.Verbose,
+			AuthenticatedWebsocketAPISupport: exch.API.AuthenticatedWebsocketSupport,
+			WebsocketTimeout:                 exch.WebsocketTrafficTimeout,
+			DefaultURL:                       bitstampWSURL,
+			ExchangeName:                     exch.Name,
+			RunningURL:                       exch.API.Endpoints.WebsocketURL,
+			Connector:                        b.WsConnect,
+			Subscriber:                       b.Subscribe,
+			UnSubscriber:                     b.Unsubscribe,
+		})
 	if err != nil {
 		return err
 	}