Conversation
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
| quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, HeaderValidator& validator, | ||
| uint32_t max_headers_allowed, absl::string_view& details) { | ||
| uint32_t max_headers_allowed, absl::string_view& details, | ||
| quic::QuicRstStreamErrorCode& rst) { |
There was a problem hiding this comment.
Instead of passing in one more argument, can we tell which reset error to use based on the output argument details? If the return value is null, details_ must have been populated already by these two utility functions.
There was a problem hiding this comment.
true, but details can arguably be pretty wide and I'd prefer to pass the argument over doing string compares
There was a problem hiding this comment.
Or the other way around, passing in rst error code and infer details according to the error code later if the returned header is nullptr?
quic::QUIC_STREAM_EXCESSIVE_LOAD => Http3ResponseCodeDetailValues::too_many_headers or Http3ResponseCodeDetailValues::too_many_trailers
quic::QUIC_BAD_APPLICATION_PAYLOAD => Http3ResponseCodeDetailValues::invalid_underscore, which should have already been populated.
There was a problem hiding this comment.
I guess I'd rather set both, than return QUIC_STREAM_EXCESSIVE_LOAD, and have to infer details in 2 different places - we can't use the same utility function for the two functions since one will be too_many_headers and the other too_many_trailers so they'd both need their own switch which would need to be kept up to date. I agree it's unfortunate to have the extra argument but I think it's cleaner than a switch statement to infer details for both functions
How about I call in @mattklein123 as a style tiebreaker - if he prefers the single argument I'll move it over tuesday when I'm back at work?
There was a problem hiding this comment.
I don't strongly prefer fewer argument in that case. Adding more and more output arguments indicates we probably need a better interface any way. But that is out of scope of this PR. We can leave it as is for now.
| end_stream_decoded_ = true; | ||
| } | ||
|
|
||
| quic::QuicRstStreamErrorCode transform_rst = quic::QUIC_STREAM_EXCESSIVE_LOAD; |
There was a problem hiding this comment.
Can the rst error be initialized to a neutral error like QUIC_STREAM_NO_ERROR in both places. I was wondering why these two were initialized with different errors when I first read the code.
There was a problem hiding this comment.
+1, this confused me also. Or a comment?
| quicHeadersToEnvoyHeaders(const quic::QuicHeaderList& header_list, HeaderValidator& validator, | ||
| uint32_t max_headers_allowed, absl::string_view& details) { | ||
| uint32_t max_headers_allowed, absl::string_view& details, | ||
| quic::QuicRstStreamErrorCode& rst) { |
There was a problem hiding this comment.
I don't strongly prefer fewer argument in that case. Adding more and more output arguments indicates we probably need a better interface any way. But that is out of scope of this PR. We can leave it as is for now.
mattklein123
left a comment
There was a problem hiding this comment.
LGTM at a high level with some quick comments from a skim.
/wait
| end_stream_decoded_ = true; | ||
| } | ||
|
|
||
| quic::QuicRstStreamErrorCode transform_rst = quic::QUIC_STREAM_EXCESSIVE_LOAD; |
There was a problem hiding this comment.
+1, this confused me also. Or a comment?
| rst); | ||
| if (trailers == nullptr) { | ||
| onStreamError(close_connection_upon_invalid_header_); | ||
| onStreamError(close_connection_upon_invalid_header_, quic::QUIC_STREAM_EXCESSIVE_LOAD); |
There was a problem hiding this comment.
| onStreamError(close_connection_upon_invalid_header_, quic::QUIC_STREAM_EXCESSIVE_LOAD); | |
| onStreamError(close_connection_upon_invalid_header_, rst); |
?
Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
|
(Ci deflaked) |
Follow up from #17520 to make the rst codes more accurate
Risk Level: low
Testing: new unit tests
Docs Changes: n/a
Release Notes: n/a