Auto-cycling of WebSocket connections - with new pre-disconnect hook - #247
Conversation
There was a problem hiding this comment.
Really enjoyed this one, hard to see a 🐛 from here, just had small code nits and tried my best to read it all.
The wsConnection abstraction is great and very useful in a future world of client-side load balancing/high availability.
Think like ffresty we need a EnableClientMetrics and some counters for all these potential failures / state transitions and timers on any sends. But I'll contribute that separately.
Test server seems correct in how to test such a complex mode of operation.
There was a problem hiding this comment.
Does this warrant a separate test file ?
|
|
||
| // New creates a new outbound client that can be connected to a remote server. | ||
| // ** Recommend using NewWithConfig directly ** | ||
| func New(ctx context.Context, config *WSConfig, beforeConnect WSPreConnectHandler, afterConnect WSPostConnectHandler) (WSClient, error) { |
There was a problem hiding this comment.
nit: not important for this PR, but using the example to hightlight that variadic functional options here would have allowed the addition of a WSPreDisconnectHandler to New without needing a separate func: https://github.com/uber-go/guide/blob/master/style.md#functional-options.
So unlike Java, in a lot of our libraries I'm trying to make sure we avoid so many constructors/funcs when one can do with lots of options.
| close(c.promoted) // sole connection - immediately the consumer of the shared send channel | ||
| w.current = c |
There was a problem hiding this comment.
is likely fine - but mutex ?
| isDemoted = true | ||
| case <-timeoutContext.Done(): | ||
| if err := c.heartbeatCheck(); err != nil { | ||
| l.Errorf("WS %s closing: %s", w.url, err) |
| // Start reading, and atomically switch all new sends over to the new connection | ||
| newC.startReader() | ||
| if !w.promoteConnection(old, newC) { | ||
| // The client was closed while we were cycling - clean up the orphaned new connection |
There was a problem hiding this comment.
Might be good to have a WARN log here ? I think that will be one of many if the client was closed while we did this, but just so we know it closed in the middle of a cycle/promotion
ca19f75 to
369f6a0
Compare
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
Signed-off-by: Peter Broadhurst <peter.broadhurst@kaleido.io>
369f6a0 to
4624acf
Compare
2 participants
WebSockets are a long-lived connection to a single server instance, often routed through multiple complex network devices/proxies.
Leaving a single connection active indefinitely has some real-world challenges:
This PR proposes a new feature usable by all users of the wsclient client, which provides managed cycling of websockets on a regular interval.
The rotation cycles is as follows:
sendactivity is routed to the new connection