Copilot commented on code in PR #13130:
URL: https://github.com/apache/trafficserver/pull/13130#discussion_r3165551078
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -975,7 +975,6 @@ void
Http2Stream::send_body(bool /* call_update ATS_UNUSED */)
{
Http2ConnectionState &connection_state = this->get_connection_state();
- _timeout.update_inactivity();
reentrancy_count++;
Review Comment:
This change alters how HTTP/2 stream inactivity is tracked under
flow-control / write-blocked conditions (e.g., `NO_WINDOW`). I couldn't find
existing gold tests that exercise the `NO_WINDOW` path (no references in
`tests/`), so it would be good to add a regression test that stalls an HTTP/2
stream via flow control and asserts `transaction_no_activity_timeout_in` fires
(and also that real outbound DATA resets the timer).
##########
src/proxy/http2/Http2Stream.cc:
##########
@@ -975,7 +975,6 @@ void
Http2Stream::send_body(bool /* call_update ATS_UNUSED */)
{
Http2ConnectionState &connection_state = this->get_connection_state();
- _timeout.update_inactivity();
reentrancy_count++;
Review Comment:
Removing `_timeout.update_inactivity()` here means the stream may no longer
bump its inactivity timer after *successful* DATA frame sends in cases where no
`signal_write_event()` is emitted (e.g.,
`Http2ConnectionState::send_data_frames()` can send one or more DATA frames,
then stop on `NO_WINDOW` while `more_data` is still true, and it does not call
`signal_write_event()` in that path). That can lead to premature
`transaction_no_activity_timeout_in` even though bytes were just sent. Consider
updating inactivity only when a frame is actually enqueued/sent (e.g.,
alongside `update_sent_count()` / after `xmit()`), rather than at the start of
`send_body()` or on every send attempt.
--
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]
