Implement Channel IDLEness

[ejona86]

We are currently lazy-initializing transports in order to allow the Channel to reach a state similar to IDLE. However, we should actively disconnect the TCP connection after inactivity in order to not rely on server idle checks and to comply with the gRPC IDLE semantics.

[zhangkun83]

There is a question about on what layer the idleness should be implemented on. It could be per-channel or per-TransporSet. I think we should do per-TransportSet, because it's generally more useful and actually required by affinity-based routing where there isn't a pre-determined set of addresses that the channel should maintain connections with, meaning that we need to close idle connections.

I propose that TransportSet keeps track of number of live streams, and shuts itself down if it has had 0 streams for IDLE_TIMEOUT. There is a race where a stream will fail if it's created on the TransportSet between it's shutdown and terminated. It can be solved by introducing the activeTransportSets collection in ManagedChannelImpl in addition to the transports collection. A TransportSet is moved out of the activeTransportSets as soon as it's shutdown, and new calls only pick TransportSets from the activeTransportSets.

[ejona86]

I think per-TransportSet would be pretty easy. Although we may want the LoadBalancer and NameResolver to go idle as well.

I propose that TransportSet keeps track of number of live streams, and shuts itself down if it has had 0 streams for IDLE_TIMEOUT.

I'd prefer not. Instead, I'd prefer the transport help out the TransportSet. The transport already is maintaining a list of the streams. The TransportSet is not currently notified of stream closure, so it would have to intercept each stream in order to be informed.

I'd also worry about having another object that could shut itself down. Each type of object that can do that adds complexity to its users.

Could the transport instead notify the TransportSet when it has zero streams and more than one stream?

A TransportSet is moved out of the activeTransportSets as soon as it's shutdown, and new calls only pick TransportSets from the activeTransportSets.

That still seems racy, since there is a time between the pick and obtainActiveTransport().

Instead could the TransportSet just have an IDLE state itself (distinct from shutdown)? That way there wouldn't be any race nor recycling of TransportSets; the users of TransportSet would need no modifications. The threading seems relatively easy because TransportSet is already lazy. Just set activeTransport to null, but without setting shutdown = true; obtainActiveTransport would Just Work™ and we'd have no additional races.

It is true we would still have a race between obtainActiveTransport() and newStream(), where we could have shut down the transport. And this is the first case where that race could introduce false failures, so we may try to fix it. But if we shut down the TransportSet we would need to fix both races.

[zhangkun83]

Although we may want the LoadBalancer and NameResolver to go idle as well.

We can mark the channel as idle when all TransportSets are idle, then close LoadBalancer and NameResolver.

I'd prefer the transport help out the TransportSet.

Yes we can do that. Make the transport notify TransportSet when its stream count becomes zero?

Instead could the TransportSet just have an IDLE state itself (distinct from shutdown)?

I was thinking about how we clean up TransportSets that are no longer needed. Now I think it can be done without TransportSet shutting down itself. We could let getTransport() clean up TransportSets that have been in IDLE for a certain time.

It is true we would still have a race between obtainActiveTransport() and newStream()

We could mark activity in obtainActiveTransport(), and reasonably assume that newStream() will be called way before IDLE_TIMEOUT is reached. Thus such race is theoretically possible but unlikely in practice.

[ejona86]

Make the transport notify TransportSet when its stream count becomes zero?

Yes, which I had mentioned. Note that for dealing with races, the transport should notify when count becomes non-zero as well.

We could let getTransport() clean up TransportSets that have been in IDLE for a certain time.

That seems like it would work, but it also seems not-quite right, at least by itself. I'd much rather have a precise way to know whether a Transport/TransportSet is considered important, which is the same goal as TransportManager.updateRetainedTransports(). We could delay a cleanup of a non-retained transport until it went idle, but I can think of some reasons to shut it down immediately. I think we need to have a separate discussion/design around this point, but that can happen later if we can come to quick consensus that it shouldn't need to to require IDLE to function.

reasonably assume that newStream() will be called way before IDLE_TIMEOUT is reached

Yes, I did consider that and think it is fair at this point. I think tackling the race at this point doesn't seem a high priority.

But I also don't want to design around the IDLE_TIMEOUT too heavily, because an IDLE_TIMEOUT of 0 should be something that we could eventually support. If for no other reason, to prevent flakiness.

[zhangkun83]

Actually, cleaning up idle (and without active transport) TransportSets is about GC'ing TransportSets whose transports are already (being) closed, thus it has nothing to do with updateRetainedTransports().

It's unclear yet what the expected behavior of updateRetainedTransports() should be. A naive though is that a retained transport(set) should not close transports actively unless the whole channel is idle. That means we will need an extra knob in TransportSet to say whether to close transport when becoming idle.

[zhangkun83]

Per offline discussion with @ejona86, here is the plan:

1. TransportSet manages its idleness bit, by listening to the active transport's idleness.
   • A transport is idle when it has zero stream.
   • A TransportSet is idle when all its transports are idle.
2. TransportSet doesn't shutdown transport when it becomes idle, at least in the first version. It may shutdown transport in some cases (e.g., an non-retained TransportSet may close transports after being idle for a certain TTL), but this is within the scope of connection management for LoadBalancer.
3. Channel goes into IDLE mode when there have been zero non-idle application-used TransportSets and zero delayed transports, for IDLE_TIMEOUT.
   • Channel needs to differentiate application-used TransportSets and the LoadBalancer-used ones. LoadBalancer may keep a live stream on its transport which as a result never becomes idle. This requires LoadBalancer to not share transports with application requests, which is currently guaranteed by C core API. Java could do the same.
4. In IDLE mode, Channel closes NameResolver, LoadBalancer and all TransportSets.
5. There is an obvious race between an idle Channel shutting down all TransportSets and application creating a new call, and the call may fail if its assigned transport is shutdown. We should try to avoid this spurious failure.