zwoop commented on code in PR #13420:
URL: https://github.com/apache/trafficserver/pull/13420#discussion_r3686924535
##########
include/proxy/http/HttpSM.h:
##########
@@ -707,13 +730,30 @@ HttpSM::get_cache_sm()
inline int
HttpSM::write_response_header_into_buffer(HTTPHdr *h, MIOBuffer *b)
{
- if (t_state.client_info.http_version == HTTPVersion(0, 9)) {
+ if (_ua.get_txn()->supports_direct_header_passing()) {
+ // Nothing lands in the buffer, so 0 keeps the tunnel's byte math honest.
+ _client_response_header_is_ready = true;
+ _direct_response_hdr_bytes = h->length_get();
+ return 0;
+ } else if (t_state.client_info.http_version == HTTPVersion(0, 9)) {
return 0;
Review Comment:
Which of the two branches do you mean? Neither looks dead to me, so I'd
rather ask than guess:
- The `supports_direct_header_passing()` branch is the new H2 path — it's
what replaces the serialize for HTTP/2 responses.
- The `HTTPVersion(0, 9)` branch below is still reachable: `HttpSM.cc:7545`
branches on `t_state.client_info.http_version == HTTP_0_9`, and `:7208` asserts
it is *not* 0.9, so the value does occur for inbound clients.
If you meant something else — e.g. that the `MIOBuffer *b` parameter is now
unused on the direct path — say so and I'll take another look.
##########
include/proxy/http2/Http2Stream.h:
##########
@@ -81,7 +81,14 @@ class Http2Stream : public ProxyTransaction
bool expect_receive_trailer() const override;
void set_expect_receive_trailer() override;
+ bool supports_direct_header_passing() const override;
+ bool is_parsed_receive_header_ready() const override;
+ const HTTPHdr *parsed_receive_header() const override;
+ bool has_pending_send_header() const override;
+
Http2ErrorCode decode_header_blocks(HpackHandle &hpack_handle, uint32_t
maximum_table_size);
+ Http2ErrorCode decode_header_blocks(HpackHandle &hpack_handle, uint32_t
maximum_table_size, const uint8_t *block,
Review Comment:
I tried again, and I'd rather not in this PR. `std::span<const uint8_t>`
does compile locally (clang, C++20), but there is currently **no** `std::span`
anywhere in `src/`, `include/`, or `plugins/` — this would be the first use in
the tree, which matches my recollection of it not surviving the older platforms
in CI.
Introducing it here means a portability question decided by a perf PR rather
than on its own merits, so I've left the `(block, block_len)` pair. Happy to
see it done tree-wide as its own change — the two-arg overload is a mechanical
conversion at that point.
--
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]
