Commit 332c46ff authored by Carl Mastrangelo's avatar Carl Mastrangelo Committed by GitHub
Browse files

netty: hide ProtocolNegotiator, and expose initial ChannelHandler

* netty: hide ProtocolNegotiator, and expose initial ChannelHandler

This change does two things: it hides the ProtocolNegotiator from
NSB, and exposes an internal "init channel" on NSB and NCB.  The
reason for the change is that PN is not a powerful enough
abstraction for internal Google use (and for some other outside
users with highly specific uses).

The new API exposes adding a ChannelHandler to the pipeline upon
registration of the channel.

To accomplish this, NettyClientTransport is modified to use
ChannelInitializer.  There is a comment explaining why it cannot
be used, but after looking at the the original discussion, I
believe the reasons for doing so are no longer applicable.

Specifically, at the time that CI was removed, there was no
WriteQueue class.  The WQ class buffers all writes and executes
them on the EventLoop.  Prior to WQ it was not the case that all
writes happened on the loop, so it could race.  If the write was
not on the loop, it would be put on the loops execution queue,
but with the CI handler as the target.  Since CI removed itself
upon registration, the write wouldn get fired on the wrong
handler.

With the additional of WQ, this is no longer a problem.  All
writes go through WQ, and only execute on the loop, so pipeline
changes are no longer racy.

...That is, except for the initial noop write.  This does still
experience the race.  If the channel is failed during
registration or connect, the lifecycle manager will fail for
differing, racy reasons.

====

To make things more uniform across NCT and NST, I have put them
both back to using CI.  I have added listeners to each of the
bootstrap futures.  I have also moved the initial write to the
CI, so that it always goes through the the buffering negotiation
handler.

Lastly, racy shutdown errors will be logged so that if multiple
callbacks try to shutdown, it will be obvious where they came
from and in which order they happened.

I am not sure how to test the raciness of this code, but I *think*
it is deterministic.  From my reading, Promises are resolved
before channel events so the first future to complete should be the
winner.  Since listeners are always added from the same thread,
and resolved by the loop, I think this forces determinism.

One last note: the negotiator has a scheme that is hard coded
after the transport has started.  This makes it impossible to
change schemes after the channel is started.  Thats okay, but it
should be a use case we knowingly prevent.  Others may want to
do something more bold than we do.
parent df925335
Loading
Loading
Loading
Loading
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Please to comment