Remove first FlowControlHandler from HTTP pipeline#128099
Conversation
Today we have a `FlowControlHandler` near the top of the Netty HTTP pipeline in order to hold back a request body while validating the request headers. This is inefficient since once we've validated the headers we can handle the body chunks as fast as they arrive, needing no more flow control. Moreover today we always fork the validation completion back onto the event loop, forcing any available chunks to be buffered in the `FlowControlHandler`. This commit moves the flow-control mechanism into `Netty4HttpHeaderValidator` itself so that we can bypass it on validated message bodies. Morever in the (common) case that validation completes immediately, e.g. because the credentials are available in cache, then with this commit we skip the flow-control-related buffering entirely.
DaveCTurner
added
>enhancement
:Distributed/Network
elasticsearchmachine
added
the
Team:Distributed Coordination (obsolete)
|
Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination) |
|
Hi @DaveCTurner, I've created a changelog YAML for you. |
|
NB this is not a must-have, it just seems like a decent improvement. If you think it's worth pursuing then I'd be inclined to strengthen |
| } else { | ||
| assert message instanceof HttpContent; | ||
| assert state == State.PASSING : state; // DROPPING releases any buffered chunks up-front | ||
| ctx.fireChannelRead(message); | ||
| ctx.fireChannelReadComplete(); // downstream will have to call read() again when it's ready |
There was a problem hiding this comment.
Since we don't care about individual chunks and in spirit of PR to process more efficiently we can compose all buffered chunks into one if there is more than one chunk.
There was a problem hiding this comment.
Yeah I did play around with that idea at first but I couldn't find a totally obvious way to combine the chunks back together (it's more than just the bytes in the request body, there's also decoder errors, and the last chunk is special, and maybe some other things too). Moreover this doesn't do anything to the hot path where we complete validation inline and then stream chunks straight through anyway, so it didn't seem worth pursuing. Maybe in a follow-up?
There was a problem hiding this comment.
Make sense. Added complexity seems unnecessary. Thanks.
DaveCTurner
added
auto-merge-without-approval
DaveCTurner
added
the
auto-merge-without-approval
|
Thanks Mikhail, I added some more tests (no production changes since your LGTM tho) |
DaveCTurner
added
auto-merge-without-approval
3 participants
Today we have a
FlowControlHandlernear the top of the Netty HTTPpipeline in order to hold back a request body while validating the
request headers. This is inefficient since once we've validated the
headers we can handle the body chunks as fast as they arrive, needing no
more flow control. Moreover today we always fork the validation
completion back onto the event loop, forcing any available chunks to be
buffered in the
FlowControlHandler.This commit moves the flow-control mechanism into
Netty4HttpHeaderValidatoritself so that we can bypass it on validatedmessage bodies. Morever in the (common) case that validation completes
immediately, e.g. because the credentials are available in cache, then
with this commit we skip the flow-control-related buffering entirely.