JosiahWI commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3637884499
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -452,11 +452,22 @@ Http2ConnectionState::rcv_headers_frame(const Http2Frame
&frame)
if (stream->trailing_header_is_possible()) {
// Don't leak the header_blocks from the initial, non-trailing headers.
ats_free(stream->header_blocks);
+ stream->header_blocks = nullptr;
Review Comment:
Setting `nullptr` is necessary semantically, because `header_blocks` might
not be touched after this if the header block can be read in-place.
##########
include/proxy/hdrs/HdrHeap.h:
##########
@@ -175,6 +175,7 @@ struct StrHeapDesc {
class HdrHeap
{
public:
+ // NOTE: also sizes an on-stack HEADERS encode buffer (2*this) in HTTP/2; a
static_assert bounds the growth.
Review Comment:
Good comment.
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -283,10 +283,16 @@ Http2Stream::main_event_handler(int event, void *edata)
Http2ErrorCode
Http2Stream::decode_header_blocks(HpackHandle &hpack_handle, uint32_t
maximum_table_size)
+{
+ return this->decode_header_blocks(hpack_handle, maximum_table_size,
header_blocks, header_blocks_length);
+}
+
+Http2ErrorCode
+Http2Stream::decode_header_blocks(HpackHandle &hpack_handle, uint32_t
maximum_table_size, const uint8_t *block, uint32_t block_len)
{
Http2ErrorCode error =
- http2_decode_header_blocks(&_receive_header, (const uint8_t
*)header_blocks, header_blocks_length, nullptr, hpack_handle,
- _trailing_header_is_possible,
maximum_table_size, this->is_outbound_connection());
+ http2_decode_header_blocks(&_receive_header, block, block_len, nullptr,
hpack_handle, _trailing_header_is_possible,
+ maximum_table_size,
this->is_outbound_connection());
Review Comment:
What's the reason for adding the 4-argument overload?
##########
src/proxy/http2/Http2ConnectionState.cc:
##########
@@ -2472,9 +2487,11 @@ Http2ConnectionState::send_headers_frame(Http2Stream
*stream)
http2_convert_header_from_1_1_to_2(send_hdr);
}
- uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just
in case
- ts::LocalBuffer local_buffer(buf_len);
- uint8_t *buf = local_buffer.data();
+ uint32_t buf_len = send_hdr->length_get() * 2; // Make it double just in case
+ // Keep typical header sets off the heap; oversized ones spill to it.
+ static_assert(HdrHeap::DEFAULT_SIZE * 2 <= 8192, "keep HEADERS encode stack
buffer within the event-thread stack budget");
+ ts::LocalBuffer<uint8_t, HdrHeap::DEFAULT_SIZE * 2> local_buffer(buf_len);
+ uint8_t *buf =
local_buffer.data();
Review Comment:
Do I understand correctly that you are increasing the buffer capacity here
to increase the chance that typical response headers will fit in the buffer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
