Channel interface needs shutdown/close

[louiscryan]

With the elimination of Service the Channel interface is now insufficient for normal use.

While ChannelImpl has a shutdown Channel does not and intercepting a ChannelImpl immediately converts it into Channel.

Closeable/AutoCloseable would be fine too

[ejona86]

The Channel interface was not modified with the removal of Service. Server was modified to no longer implement Service. We discussed whether to have shutdown as part of the Server interface, and decided that if we needed it, we would add it.

The general expectation is that code that creates the Channel would be in control of its lifecycle. You really shouldn't have generic helper functions creating Channel, since in the future we expect some important behavioral methods on the builder (such as idle behavior).

Given that we have a separate interface for Interceptors, we could add start/shutdown to Channel without too much problem I believe.

[ejona86]

This issue keeps coming up, and I have discussed offline with people my concerns with shutdown() on Channel. Writing a bit of it here for future sharing.

To start, I'm not sure of any use case that strongly needs this feature. It seems that the code creating the ChannelImpl just needs to hold on to a reference. There should be no issues for helper utilities, as they should modify the ChannelBuilder before construction or chain interceptors afterward; there doesn't seem to be a need for them to use or return ChannelImpl directly.

My main concern with shutdown() on Channel is that is breaks knowing what is shutdown(). With interceptors and stubs, you form "trees" of Channels with leaves being Channels/stubs used directly and the root being a ChannelImpl. Because of the many Channels that all use the same ChannelImpl, you can't predict if other Channels will break if you shutdown() another Channel.

To make it more clear, an example:

ChannelImpl channelImpl = NettyChannelBuilder.forAddress("example.com", 443).build();
Channel channel1 = ClientInterceptors.intercept(channelImpl,
    new ClientAuthInterceptor(creds1, executor));
Channel channel2 = ClientInterceptors.intercept(channelImpl,
    new ClientAuthInterceptor(creds2, executor));
channel2.shutdown();
// Now channel1 is also shutdown

Note that that is assuming ClientInterceptors can't implement shutdown(). If they can (or if someone made an interceptor implementing Channel directly), then the problem becomes more acute as now the interceptor for channel1 could be expecting shutdown() to be called. Such a design would require all channels to be shutdown(), even though all Channels with the same root would be "shutdown" as soon as the first one was. This leads to indeterminate states where the ClientInterceptor hasn't been shutdown but the underlying ChannelImpl has. Reference counting could possibly help, but it seems like we are creating more problems than we are solving.

Channels are expected to be injected (manual, or automatic) in many cases. When injected, you ideally wouldn't want them to have shutdown() because the code receiving the object doesn't know if anyone else is using it.

Basically, only the creator of the ChannelImpl knows the its appropriate lifecycle. Although it would be convenient in simple examples to not have to store a reference to the ChannelImpl, it would only work well when you know there is only one interceptor chain. However, it is explicitly part of the interceptor design to permit the "trees" to form. Permitting and encouraging users to shutdown() a Channel would seem to be a disservice to them.

[wrwg]

I think some of the confusion comes from the naming. A class called FooImpl indicates for me to have its main purpose in implementing interface Foo. That leads me to think I can just work with the Foo.

A better naming may make the design clearer. Channel is the thing on which you can initiate a call and which allows interceptors to plug in. ChannelImpl might be better called TransportChannel, ConnectedChannel, or something which indicates its a resource. That may make it more obvious you have to take care of it for clean up.

[zhangkun83]

@wrwg I feel the same. +1 for renaming ChannelImpl.

[ejona86]

I'm game for a rename to. The only reason it's been ChannelImpl is lack of a better name. TransportChannel and ConnectedChannel are both worse than ChannelImpl, in my mind. DefaultChannel seems little better than ChannelImpl.

[zhangkun83]

IMO DefaultChannel is as bad as ChannelImpl. "Default" is not conveying anything useful (default in what sense?), other than indicating it's the "default impl". Since ClientTransport is "encapsulating a single connection" (quoted from its javadoc), and ChannelImpl is based on a ClientTransport, "TransportChannel" and "ConnectionChannel" are more informative and less misleading.

[nmittler]

I think the problem here is that the term "channel" ~= "transport", but "interceptor" != "transport". I think it would be better to provide a more abstract interface that is responsible for creating calls and then all of these "things" would implement/extend that. Something like this:

interface CallFactory {
   Call newCall(MethodDescriptor method);
}

class MyInterceptor implements CallFactory {
    ...
}

interface Channel extends CallFactory {
    ...
    void shutdown();
}

class ChannelImpl implements Channel {
    ...
}

[ejona86]

ConnectionChannel and TransportChannel are worse than DefaultChannel and ChannelImpl in my mind. Connection, Transport, and Channel in many conversations (obviously not ours) are interchangeable. Connection and Transport don't add any additional meaning to Channel, yet it isn't obviously filler. When you see Default* or *Impl you can quickly ignore the filler.

We only have one Channel implementation. If we ever have more than one, then maybe this problem would be solved with SimpleChannel, LoadBalancedChannel, etc. However, I still see the value of only having one implementation of Channel. If we stay with one, maybe we rename Channel to CallFactory and rename ChannelImpl to Channel.

[nmittler]

@ejona86 I don't see the harm in having an interface for Channel even though there is only (currently) a single instance. The broader point that I was trying to make is that thinking of "interceptors" as kinds of "channels" is wrong, since they do not represent the transport ... they only represent the ability to create new calls (i.e. on an existing transport). If we can separate out a better interface name (e.g. CallFactory as suggested above), then Channel (whether it be an interface or concrete class) can have shutdown.

[ejona86]

@nmittler, the harm is that there is no distinction between Channel the interface and Channel the implementation. If there is only one implementation with no additional methods compared to the interface, then why bother having the interface? Even in tests we would want users to use the real Channel implementation.

We have trouble coming up with a name because they represent the same identical idea. If there were multiple implementations, then the name becomes whatever is special about that implementation. With only one implementation, then you get something like DefaultChannel or ChannelImpl. As seen in my previous comment, if there are multiple implementations naming them is easy.

[wrwg]

I agree with @nmittler's direction. If we consider channel and transport as too easily mixed up notions for users, perhaps the best call is to rename the interface Channel into something more neutral, like CallFactory.

In the process we could get rid of the Channel interface and rename ChannelImpl to Channel.