http: fixing a bug with HTTP/1.0 keep-alive. #9843
Conversation
Signed-off-by: Alyssa Wilk <[email protected]>
|
This is all well and good but I realized just before sending this out we still have the inverse problem. If there is a keep-alive on the request and no content length on the response, we need to frame with connection close, and the HCM does not see the connection: close (which is handled by the codec) and won't close the connection. @mattklein123 I'd like your thoughts on this. |
Where is this handled by the codec? I don't see it.
So my first reaction to looking at the diff (before I read this comment) was that we are now doing logic in the codec that has been traditionally done by the HCM, so we are on the same page that it is confusing. My preference would definitely be to have it be done one place or the other. The thing I'm not completely clear on is why you think that we can't just do this change up in the HCM vs. the codec? |
|
Since we keep missing each other.... This decision about connection close is based on how the HTTP/1 (codec-specific) content is framed. That depends on a variety of factors and has always been decided in the HTTP/1 codec. For HTTP/1 we decide if it's chunked or we have or can add content length. For HTTP/1.0 and below it's either use content length or frame-by-connection close. The fact that the HCM is currently the decider of drain/close status is what makes this case special. I think for #9846 we're going to want the codec to be able to force connection close (for codec "send direct response) so I'm inclined to keep the "how do we frame the response" logic in the codec, and either add the close-if-necessary logic there, or make it so the codec can signal the HCM, since we'll need something like that for #9846 anyway. WDYT? |
I just spent some time looking at the code and refreshing myself on the topic. My general feeling remains that I think we should handle this in the HCM. We already look for the lack of keep-alive for HTTP/1.0 connections and handle that in the HCM. Couldn't we just remember the inverse (if we saw it), and copy the header out on the response? In that case we could either check for content-length, etc., or potentially have the codec modify the connection header -> close if it needs to? I agree we could go either way here, but in general my preference would be to try to consolidate the logic if possible. I do agree that the codec will need a way to signal to the HCM that it wants to close the connection per #9846 but it seems like that could be handled via throwing some type of exception like we do for flooding, protocol, etc.? I will defer to whatever you want to do here but that's my thinking on this. |
|
Yeah, we can move it to the HCM, but if we want full consistency, we'd have to track if this is a response to a head request in both places, if status code is in the "no body" range in both places, and check for preexisting content length in both places. Alternately we could just not reuse connections in that case where we append content length 0 in the codec, and say if upstream didn't send the content length themselves they lose connection reuse. I'd be inclined to go for this since it's far less kludgy. If this SGTY I'm fine moving a simpler version of "always echo keep-alive iff content length, otherwise connection close" for HTTP/1.0 to the HCM. |
+1 sounds great. IMO we should shoot for "it is spec compliant but not guaranteed to be optimally fast" HTTP/1.0 support. |
Signed-off-by: Alyssa Wilk <[email protected]>
| @@ -560,7 +560,8 @@ TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLws) { | |||
| "GET / HTTP/1.0\r\nHost: foo.com \r\nConnection:Keep-alive\r\n\r\n", | |||
| &response, true); | |||
| EXPECT_THAT(response, HasSubstr("HTTP/1.0 200 OK\r\n")); | |||
| EXPECT_THAT(response, Not(HasSubstr("connection: close"))); | |||
There was a problem hiding this comment.
This removal reflects the fact we're now invoking the HCM drain path, which is all to the good.
We could tweak the HCM drain path to not append connection: close for HTTP/1.0 (since it's implicit) but honestly I prefer the increased clarity and minimized complexity at the cost of those extra bytes on the wire.
There was a problem hiding this comment.
nit: maybe explicitly test for presence of the header?
| @@ -560,7 +560,8 @@ TEST_P(IntegrationTest, Http10WithHostandKeepAliveAndLws) { | |||
| "GET / HTTP/1.0\r\nHost: foo.com \r\nConnection:Keep-alive\r\n\r\n", | |||
| &response, true); | |||
| EXPECT_THAT(response, HasSubstr("HTTP/1.0 200 OK\r\n")); | |||
| EXPECT_THAT(response, Not(HasSubstr("connection: close"))); | |||
There was a problem hiding this comment.
nit: maybe explicitly test for presence of the header?
Signed-off-by: Alyssa Wilk <[email protected]>
Previously Envoy would keep alive HTTP connections without sending an explicit keep-alive header.
Now if the connection isn't going to be closed and a content length is present, it explicitly sends a keep-alive header and if the connection will be closed it invokes the HCM drain-and-close logic.
Risk Level: Medium (L7 change. HTTP 1.0 only)
Testing: Integration test.
Docs Changes: n/a
Release Notes:
Fixes #9797