WIP: Have http and tchannel transports optionally take in a net.Listener

[bufdev]

This is just a concept, feel free to reject. This is re: #866, I would also add this to grpc if we thought this was the way to go.

Note this is technically a breaking change because Serve(net.Listener) error is added to tchannel.Channel. I don't know if this is OK, but if not, I can make a ChannelExtended interface or such that includes this method, and we can change this in 2.0.

[mention-bot]

@peter-edge, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kriskowal to be a potential reviewer.

[bufdev]

Putting a hold on this - I'm having some issues with TChannel when I pass in a listener to the Channel instead of an address, I've been playing with it for way too long locally.

[kriskowal]

@peter-edge I think adding ListenerChannel as an option would be a fine solution.

Note that only tchannel.ChannelTransport uses the WithChannel option. You have more control over the underlying channel for tchannel.Transport.

You can use a private interface or even bind directly to the tchannel Channel internally, since that channel is not exposed to or provided by the user.

[bufdev]

Can you give an example? Not sure I completely understand :)

…

On Tue, Apr 11, 2017 at 2:08 AM Kris Kowal ***@***.***> wrote: @peter-edge <https://github.com/peter-edge> I think adding ListenerChannel as an option would be a fine solution. Note that only tchannel.ChannelTransport uses the WithChannel option. You have more control over the underlying channel for tchannel.Transport. You can use a private interface or even bind directly to the tchannel Channel internally, since that channel is not exposed to or provided by the user. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#897 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AECGvKRtuJwpC1O9eoeKq25eoVBAsTB4ks5rusRfgaJpZM4M3YtY> .

[kriskowal]

There are currently two TChannel transport implementations, but they share the same options and config type for the sake of using the same package and names.

• ChannelTransport was the first implementation, and it supports threading an existing TChannel Channel (the interface we expose). This implementation is friendly to gradual migration to YARPC, where you can use an existing channel using TChannel RPC features for some registered handlers and configured callers. We renamed it from Transport to ChannelTransport before we cut 1.0 because we had to ship with TChannel support, but didn’t manage to ship YARPC’s load balancer abstraction to TChannel until last quarter.

• Transport is the new implementation that supports peer choosers. It does not support threading a Channel because we had to subvert the TChannel subchannel handler to take over the load balancing concern and accept requests for any service name.

Consequently, although you can provide a WithChannel and ServiceName option to both NewTransport and NewChannelTransport, only the latter uses them.

What this implies for this change:

In this change you’re breaking backward compatibility by adding a Serve method to the Channel interface. We use that interface in two different ways. One is as an option for NewChannelTransport. The other is for a private property of Transport. You could introduce a private listenChannel interface that would not interfere with the old ChannelTransport options.

[bufdev]

Closing this for now, may revisit at another time