bneradt commented on code in PR #9987:
URL: https://github.com/apache/trafficserver/pull/9987#discussion_r1453873309
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -315,19 +309,36 @@ Http2Stream::send_request(Http2ConnectionState &cstate)
}
} while (!done);
- if (bufindex == 0) {
+ if (dumpoffset == 0) {
// No data to signal read event
return;
}
// Is the _sm ready to process the header?
if (this->read_vio.nbytes > 0) {
if (this->receive_end_stream) {
- this->read_vio.nbytes = bufindex;
- this->read_vio.ndone = bufindex;
+ // This is dangerous. The _sm has possibly not read
+ // the data yet, so we end up with a mismatch if there
+ // is something async in between.
+ // Better would be to get the actual difference between
+ // the start of the headers and the actual position in
+ // the buffer for the ndone part.
+ // Howerver, during testing we have not seen any issues.
+ this->read_vio.nbytes = this->read_vio.ndone + dumpoffset;
if (this->is_outbound_connection()) {
+ // This is a response trailer.
+ // We don't set ndone because the VC_EVENT_EOS will
+ // first flush the remaining content to consumers,
+ // after which the TUNNEL_EVENT_DONE will be fired
+ // and the trailer handler will be set up.
+ // The trailer handler will read the buffer, and not
+ // get its content from the VIO
+ // This can break if the implementation
+ // changes.
this->signal_read_event(VC_EVENT_EOS);
} else {
+ // Client trailing headers.
+ this->read_vio.ndone = this->read_vio.nbytes;
Review Comment:
How do we know these are trailers rather than the initial HTTP/2 headers?
Should we guard these around `if (this->trailing_header_is_possible()) {`?
--
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]
