Keepalive support #1992
Keepalive support #1992
Conversation
|
The integration part with OkhttpClientTransport is not done yet. I will do it in another commit or pull request. |
| } | ||
|
|
||
| public KeepAliveManager(ManagedClientTransport transport, ScheduledExecutorService scheduler) { | ||
| this(transport, scheduler, SYSTEM_TICKER, KEEPALIVE_DELAY, KEEPALIVE_TIMEOUT); |
There was a problem hiding this comment.
The keepalive delay must be configurable.
There was a problem hiding this comment.
Added another constructor.
|
Updated okhttp transport to use keepalive. |
| // We don't have any active streams. No need to do keepalives any more. | ||
| // Again, we have to call this inside the lock to avoid the race between onTransportIdle | ||
| // and onTransportActive. | ||
| keepAliveManager.onTransportIdle(); |
There was a problem hiding this comment.
This can be done in maybeClearInUse() instead, before calling listener.transportInUse(false).
There was a problem hiding this comment.
I see both maybeClearInUse and setInUse actually consider pending streams. IIUC, when there's no connection, all streams will be pending streams. We probably don't want to do keepalives when there're only pending streams.
There was a problem hiding this comment.
Pending streams are a bit of an edge case. Pending streams used to happen before transportReady() (but that's no longer the case due to changes in TransportSet). Otherwise pendingStreams are only caused by MAX_CONCURRENT_STREAMS. The only case we could have pendingStreams but no active streams is when MAX_CONCURRENT_STREAMS is 0. It may be a good idea to do pings in that case, since if the connection goes down we'll be permanently hosed as we'll never do writes (which means we may never detect the closure).
There was a problem hiding this comment.
I see. Changed it to what Kun suggested.
|
I find |
|
@ejona86 is concerned about the complexity of ResetttableTimer. @carl-mastrangelo suggests that we periodically check the time of last activity, and schedule a ping if it was older than |
| /** | ||
| * Enable keepalive with default delay and timeout. | ||
| */ | ||
| @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1785") |
There was a problem hiding this comment.
No @ExperimentalApi for package-private classes.
|
@zsurocking LGTM |
Creates a KeepAliveManager which should be used by each transport. It does keepalive pings and shuts down the transport if does not receive an response in time. It prevents the connection being shut down for long lived streams. It could also detect broken socket in certain platforms.
Creates a KeepAliveManager which should be used by each transport. It does keepalive pings and shuts down the transport if does not receive an response in time.
It prevents the connection being shut down for long lived streams. It could also detect broken socket in certain platforms.