Add heartbeat ACK response and error handling #396
Conversation
- Error when sending a heartbeat now triggers a reconnection - Op7 now triggers a reconnection - Session now tracks the last heartbeat ACK that was recieved. If the last ACK is more than FailedHeartbeatAcks*heartbeatinterval in the past, this is treated as a dead connection and a reconnection is forced.
wsapi.go
Outdated
| s.Lock() | ||
| s.DataReady = false | ||
| s.Unlock() | ||
| if err != nil || time.Now().UTC().Sub(last) > (i*FailedHeartbeatAcks*time.Millisecond) { |
There was a problem hiding this comment.
Time.Sub() returns time in nanoseconds. At a glance, multiplying by time.Milliseconds looks wrong. Is there a way that you can make this i*FailedHeartbeatAcks and pull up the Millisecond value up into the constant, also verify/test this works.
There was a problem hiding this comment.
This has been tested and it works. The reason is that because this is a duration, multiplying by Millisecond actually turns converts the value into the appropriate number of nanoseconds, which is why you can compare any duration to any other duration and always have it work (because they're ALL represented in nanoseconds). I can pull the millisecond amount up into the constant for readability, though.
There was a problem hiding this comment.
Hmm, looks like i is not in nanoseconds, we're just Unmarshalling into it from a msec value.
Your change seems right, but would you mind renaming the argument in this method to heartbeatIntervalMsec instead of i, thanks so much.
| @@ -215,16 +219,22 @@ func (s *Session) heartbeat(wsConn *websocket.Conn, listening <-chan interface{} | |||
| defer ticker.Stop() | |||
|
|
|||
| for { | |||
| s.RLock() | |||
There was a problem hiding this comment.
You need a write lock here.
There was a problem hiding this comment.
This is not writing anything, it's reading a value. Why would i need a write lock?
wsapi.go
Outdated
| s.log(LogWarning, "error closing session connection, %s", err) | ||
| } | ||
|
|
||
| s.log(LogInformational, "calling reconnect() now") |
There was a problem hiding this comment.
Not sure this is needed, the log above seems like enough.
|
Thanks for the PR! |
* Add heartbeat ACK response and error handling - Error when sending a heartbeat now triggers a reconnection - Op7 now triggers a reconnection - Session now tracks the last heartbeat ACK that was recieved. If the last ACK is more than FailedHeartbeatAcks*heartbeatinterval in the past, this is treated as a dead connection and a reconnection is forced. * Address @iopred comments
last ACK is more than FailedHeartbeatAcks*heartbeatinterval in the past,
this is treated as a dead connection and a reconnection is forced.