bneradt commented on code in PR #9987:
URL: https://github.com/apache/trafficserver/pull/9987#discussion_r1453918662
##########
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;
Review Comment:
Looking at this a bit, the HttpSM sets up a tunnel for the body bytes
_after_ processing the headers. Therefore the nbytes here for that tunnel will
be processing just the HTTP/2 DATA payloads and the trailers, without
processing the original HTTP/2 headers. Conveniently, `Http2Stream` already
keeps track of HTTP/2 DATA frame payloads via the `data_length` member. Taken
together, I think we can accomplish the explicit calculation that @maskit is
advocating for simply by using that member and `dumpoffset`:
```cpp
// These headers may be standard or trailer headers:
//
// * If they are standard, then there is no body (note again that the
// END_STREAM flag was sent with them), data_length will be 0, and
// dumpoffset will simply be the length of the headers.
//
// * If they are trailers, then the tunnel behind the SM was set up
after
// the original headers were sent, and thus nbytes should not include
the
// size of the original standard headers. Rather, for trailers, nbytes
// only needs to include the body length (i.e., DATA frame payload
// length), and the length of these current trailer headers calculated
in
// dumpoffset.
this->read_vio.nbytes = this->data_length + dumpoffset;
```
--
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: github-unsubscr...@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
