-
Notifications
You must be signed in to change notification settings - Fork 690
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
Add websocket keepalive #50
Conversation
Codecov Report
@@ Coverage Diff @@
## master #50 +/- ##
==========================================
- Coverage 82.2% 80.46% -1.74%
==========================================
Files 17 17
Lines 927 947 +20
==========================================
Hits 762 762
- Misses 102 122 +20
Partials 63 63
Continue to review full report at Codecov.
|
websocket.go
Outdated
go func() { | ||
defer ticker.Stop() | ||
for { | ||
err := c.WriteMessage(websocket.PingMessage, []byte("keepalive")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use c.WriteControl to send a ping message without data: https://godoc.org/github.com/gorilla/websocket#Conn.WriteControl
websocket.go
Outdated
@@ -35,6 +38,7 @@ var wsServe = func(cfg *wsConfig, handler WsHandler, errHandler ErrHandler) (don | |||
} | |||
}() | |||
defer close(doneC) | |||
keepAlive(c, cfg.timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's better to enable keepAlive as an config option, may be we should disable it by default
websocket_service.go
Outdated
@@ -24,7 +26,7 @@ type WsPartialDepthHandler func(event *WsPartialDepthEvent) | |||
// WsPartialDepthServe serve websocket partial depth handler with a symbol | |||
func WsPartialDepthServe(symbol string, levels string, handler WsPartialDepthHandler, errHandler ErrHandler) (doneC, stopC chan struct{}, err error) { | |||
endpoint := fmt.Sprintf("%s/%s@depth%s", baseURL, strings.ToLower(symbol), levels) | |||
cfg := newWsConfig(endpoint) | |||
cfg := newWsConfig(endpoint, timeout) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if timeout cannot be changed, it's not a good idea to pass it as a required argument to newWsConfig
, use optional argument instead?
Made keepalive optional. I didn't want to break the api, so I added two package-level variables. |
My attempt to implement the ping-pong keepalive mechanism. Since I didn't want to break the compatibility, the timeout is set to constant 60 seconds.