http: http/1 codec refactor #10199
http: http/1 codec refactor #10199
Conversation
These changes are required for fully splitting the header map into discrete types. 1) Prepare for concrete types 2) Fully remove client pipelining support as we don't actually use it anywhere and it made the refactor more complicated. 3) As a bonus, remove per-request/response heap allocations for HTTP/1. Signed-off-by: Matt Klein <mklein@lyft.com>
| @@ -1155,7 +1155,7 @@ TEST_F(Http1ServerConnectionImplTest, IgnoreUpgradeH2cCloseEtc) { | |||
| TestHeaderMapImpl expected_headers{{":authority", "www.somewhere.com"}, | |||
| {":path", "/"}, | |||
| {":method", "GET"}, | |||
| {"connection", "Close,Etc"}}; | |||
There was a problem hiding this comment.
This change is due to the prod code change I pointed out above. Let me know what you think.
There was a problem hiding this comment.
We could summon @PiotrSikora ?
|
@asraa @alyssawilk PTAL thank you! |
| @@ -562,16 +562,6 @@ int ConnectionImpl::onHeadersCompleteBase() { | |||
| ENVOY_CONN_LOG(trace, "codec entering upgrade mode.", connection_); | |||
There was a problem hiding this comment.
Gotcha, perhaps the PR author assumed full connection sanitization was happening in the h2c case, but it only removed upgrade/http2settings from connection header. Speaking of, can you make the quick change from
current_headers.remove(Headers::get().Http2Settings);
current_headers.removeHttp2Settings();
There was a problem hiding this comment.
Http2Settings is not an O(1) header currently?
There was a problem hiding this comment.
Oh, maybe it's not that significant, you just don't have to do the lookup at all if you use remove##name.
| TestRequestHeaderMapImpl headers{{":method", "GET"}, {":path", "/"}, {":authority", "host"}}; | ||
| request_encoder->encodeHeaders(headers, true); | ||
| EXPECT_EQ("GET / HTTP/1.1\r\nhost: host\r\ncontent-length: 0\r\n\r\n", output); | ||
| output.clear(); | ||
|
|
||
| // 2nd pipeline request. |
There was a problem hiding this comment.
For my own knowledge, why was this removed in this PR?
There was a problem hiding this comment.
When the header maps are fully split from an interface perspective, the existing code did not work very well as we had the wrong header type at various places in the base class. This was especially problematic with HEAD requests. I could have probably fixed this a different way, but I realized that if I completely removed pipelining support, fixing became a lot easier. This also removes all heap allocations for HTTP/1 requests and responses which is a pretty nice perf benefit. Given that we don't actually use pipelining anywhere in the real code, this seemed like a good simplification to make.
BTW this is why I split this change out into its own PR because it definitely needs a good review.
Signed-off-by: Matt Klein <mklein@lyft.com>
Signed-off-by: Matt Klein <mklein@lyft.com>
| @@ -1155,7 +1155,7 @@ TEST_F(Http1ServerConnectionImplTest, IgnoreUpgradeH2cCloseEtc) { | |||
| TestHeaderMapImpl expected_headers{{":authority", "www.somewhere.com"}, | |||
| {":path", "/"}, | |||
| {":method", "GET"}, | |||
| {"connection", "Close,Etc"}}; | |||
There was a problem hiding this comment.
We could summon @PiotrSikora ?
Signed-off-by: Matt Klein <mklein@lyft.com>
|
@alyssawilk updated PTAL |
|
/azp run envoy-linux |
|
Azure Pipelines successfully started running 1 pipeline(s). |
These changes are required for fully splitting the header map into
discrete types.
anywhere and it made the refactor more complicated.
Risk Level: Medium
Testing: Existing tests (had to modify one, see my comment in the review)
Docs Changes: N/A
Release Notes: N/A