core,netty: Add server negotiation timeout before client preface #3670
Conversation
ejona86
added
the
TODO:release blocker
| * @since 1.8.0 | ||
| */ | ||
| @ExperimentalApi // FIXME | ||
| public T negotiationTimeout(long timeout, TimeUnit unit) { |
There was a problem hiding this comment.
Should we log a warning when it's unsupported?
There was a problem hiding this comment.
I'd either want to throw or do nothing. I'd be fine swapping to throwing.
There was a problem hiding this comment.
Swapped to throwing. Doesn't really change much since all our transports receive an implementation from AbstractServerImplBuilder.
| } | ||
| return new ServerTransportListenerImpl(transport); | ||
| ServerTransportListenerImpl stli = new ServerTransportListenerImpl(transport); | ||
| stli.init(); |
There was a problem hiding this comment.
Is this the precise point that protocol negotiation starts, and transportReady the precise point that it ends? InProcessServer also does this? Does it make sense?
There was a problem hiding this comment.
At this point a TCP connection has been established, but nothing else. transportReady is called before any RPCs are triggered.
zpencer
left a comment
zpencer
left a comment
There was a problem hiding this comment.
lgtm with throwing change
|
PTAL In addition to throwing, I renamed to |
| initChannel(new GrpcHttp2ServerHeadersDecoder(GrpcUtil.DEFAULT_MAX_HEADER_LIST_SIZE)); | ||
|
|
||
| handler().handleProtocolNegotiationCompleted(Attributes.EMPTY); | ||
| verify(transportListener, never()).transportReady(any(Attributes.class)); |
There was a problem hiding this comment.
They're equivalent here. But isA would actually be worse, because if I had the wrong class I could miss callbacks that I wanted to trigger failure. I'm not trying to verify it is an Attributes; I just checking the method which happens to have certain arguments.
There was a problem hiding this comment.
Oh, this verify checks never(), I should have meant the second verify.
There was a problem hiding this comment.
Yeah, the second verify would fall into "I'm not trying to verify it is an Attributes".
Negotation is the period between transport creation and ready.
This mirrors the behavior of client-side.
ejona86
force-pushed
the
server-negotiation-timeout
branch
from
732ec44 to
9d5808b
Compare
ejona86
added
the
TODO:backport
ejona86
removed
TODO:release blocker
3 participants
The two commits here are easiest to review separately and will be committed separately. The first adds support for a negotiation timeout to core and the second improves transportReady callback placement in Netty so the negotiation timeout applies to more phases.
Before merging I'll reserve an issue for
negotiationTimeout's@ExperimentalApiand update the annotation.