[http] Move HTTP1 request flood checks from response encode to request decode.#10475
Conversation
Decode of a new request is guaranteed to happen on a shallow call stack under HCM, which makes it a better choice for flood checks since it reduces the number of unique call stacks under which FrameFloodException may be thrown. Signed-off-by: Antonio Vicente <avd@google.com>
Signed-off-by: Antonio Vicente <avd@google.com>
alyssawilk
left a comment
There was a problem hiding this comment.
Cool, I think moving this is much simpler than the try / catch PR, but I think you need to update some comments and probably docs. The logs, thrown error message and stats all talk about too many responses, where now we're reacting to an incoming request when there are too many responses.
|
+1 I prefer this solution. I think we will need to make similar changes to HTTP/2 but I'm not sure? cc @yanavlasov. |
Increase the number of requests processed by IntegrationTest.TestManyBadRequests Signed-off-by: Antonio Vicente <avd@google.com>
I went through "git show f5b0294" and updated relevant comments and exception strings. |
The "throws FrameFloodException" in the H2 codec are protected by either dispatching_downstream_data_ or dispatching_, so I think uncaught exception shouldn't be a problem. I need to look into this further but I think we have the problem of incrementOutboundFrameCount only throwing when dispatching_downstream_data_ is true. Calls to ConnectionImpl::addOutboundFrameFragment under an upstream read callback will buffer infinitely (up to flow control limits). |
mattklein123
left a comment
There was a problem hiding this comment.
Awesome, thanks. Will defer to @alyssawilk for further review.
alyssawilk
left a comment
There was a problem hiding this comment.
hm, I thought it was worth updating more docs but honestly I think this is clear enough. One fix to (prior) comments and you're good to go!
|
@antoniovicente it looks like on TSAN /wait |
… connection backs up and the response flood can be detected in IntegrationTest.TestFloodUpstreamErrors Signed-off-by: Antonio Vicente <avd@google.com>
ce1cb23
It is a consistent failure. The test case I added takes requires processing over a thousand tiny responses before it fills the TCP connection buffer. Setting SO_RCVBUF to 1024 on the client connection reduces the time until the socket backs up. In opt mode the test case runs in about 2 seconds, but under tsan it takes 20 seconds for each of ipv4/ipv6 (down from 120 secs each before the SO_RCVBUF change). |
Description: Move HTTP1 request flood checks from response encode to request decode.
Decode of a new request is guaranteed to happen on a shallow call stack under HCM, which makes it a better choice for flood checks since it reduces the number of unique call stacks under which FrameFloodException may be thrown.
Fixes several cases where FrameFloodException was thrown outside a try/catch block, resulting in an Envoy crash.
Risk Level: low
Testing: unit and integration
Docs Changes: n/a
Release Notes: n/a