Add automatic reconnect to the client connection. #489
Conversation
Sources/GRPC/ClientConnection.swift
Outdated
| /// - Parameter configuration: The configuration to start the connection with. | ||
| public class func start(_ configuration: Configuration) -> EventLoopFuture<ClientConnection> { | ||
| return start(configuration, backoffIterator: configuration.connectionBackoff?.makeIterator()) | ||
| /// The `EventLoop` this connection is using. Note that this _may_ change over time. |
There was a problem hiding this comment.
Not a fan of allowing this to change; could we re-use the previous channel's event loop instead?
There was a problem hiding this comment.
Do you mean re-use it for the new channel? Or store it and always return it here? Because the former isn't possible.
There was a problem hiding this comment.
I was hoping for the former; why would that be a problem?
(My concern is that Vapor does a lot of thread-local stuff and generally expects all "sub-requests" to run on the same event loop the original request is handled on. Open to keep this as-is for now, though, given that one could simply pass in the request's EventLoop as the client's EventLoopGroup argument.)
There was a problem hiding this comment.
I take that back -- I didn't realise EventLoop conformed to EventLoopGroup!
| /// - Parameter state: The state on which to call the given callback. | ||
| /// - Parameter callback: The closure to call once the given state has been transitioned to. The | ||
| /// `callback` can be removed by passing in `nil`. | ||
| public func onNext(state: ConnectivityState, callback: Callback?) { |
There was a problem hiding this comment.
What is this for as opposed to just providing a delegate, and why is the callback reset after every change to that state?
There was a problem hiding this comment.
Good question; my take on it is that they're useful for different periods of time. You can achieve exactly the same with a delegate but in some cases this is just much more convenient (the tests are a good example of this). The delegate is useful for longer time periods (logging/monitoring). This is useful for single events (e.g. my connection has shutdown, I can now update my UI) which is why it resets after each change. Definitely open to not resetting after each change though as I realise that's a slightly strange decision!
There was a problem hiding this comment.
Also, if we don't reset, we could have the user pass in a closure instead that could switch over the states they are interested in and discard the rest via default:.
At that point, one could also argue over removing the delegate and only letting the user provide callback closures. And at that point we could even keep an array of closures, in case the user wants more than one observer.
To be honest, having two different ways of passing in callback closures feels a bit weird right now, but this is not a blocker. We can revisit it later (before 1.0).
| self.wait(for: [reconnectionReady], timeout: 1.0) | ||
| XCTAssertEqual(self.stateDelegate.clearStates(), [.connecting, .ready]) | ||
|
|
||
| // Ensure we can actually make a call. |
There was a problem hiding this comment.
Could we also test making a call after the first server has shut down, but before the second one has been started?
There was a problem hiding this comment.
We can, but the reconnect will already be taking place: if we wait() on that call to complete (before starting the server) it will eventually time out the reconnect (and cause a .shutdown).
If we don't wait() on it, then it will eventually connect and that call will succeed.
I'll add the former as a separate test.
There was a problem hiding this comment.
If we don't wait() on it, then it will eventually connect and that call will succeed.
I think that's what we should do — start the call while the server is offline, then wait() for it to succeed after the server is back up.
Motivation: Clients could lose their connections to the server for a number of reasons, we offered no means to automatically reconnect. Modification: - `ClientConnection` now stores a future Channel and future multiplexer, as opposed to future `ClientConnection`s holding a channel and multiplexer. - This allows us to replace the future channel and future multiplexer when the client is closed (but not explicitly via `close()`). - The state can be monitored via a delegate or by registering callbacks on the next transition to a given state. - Reconnection uses the same backoff logic used for the initial connection creation. Results: Clients can automatically reconnect when their connection goes away.
2 participants
Motivation:
Clients could lose their connections to the server for a number of
reasons, we offered no means to automatically reconnect.
Modification:
ClientConnectionnow stores a future Channel and future multiplexer,as opposed to future
ClientConnections holding a channel andmultiplexer.
when the client is closed (but not explicitly via
close()).on the next transition to a given state.
connection creation.
Results:
Clients can automatically reconnect when their connection goes away.