bneradt commented on PR #13780:
URL: https://github.com/apache/trafficserver/pull/13780#issuecomment-5942124579
Thanks for the thorough review, @bryancall. I force-pushed an updated
commit. Point by point:
**Pure END_STREAM / flag consumed before delivery.** Fixed as you suggested.
`_read_complete_deferred` is now `int _deferred_read_event`. On re-enable the
stream schedules that same event with `send_tracked_event`, and clears it only
on that hand-off (when `_sm` and `read_vio.cont` are set). It no longer goes
through `update_read_request`, so it isn't re-derived from `nbytes` and none of
that function's early returns can drop it. Thread discipline holds:
`main_event_handler` calls `_switch_thread_if_not_on_right_thread` when the
event lands. I did not apply Copilot's `set_read_done()` suggestion.
One thing I found while testing: on an outbound stream whose request has
already ended (any GET), an empty END_STREAM DATA frame moves the stream to
CLOSED, so it takes the `initiating_close()` branch above, not
`signal_final_read_event()`. That EOS is a tracked event that isn't gated by
`is_disabled()`, so it completed even before this change. The deferral path you
identified only applies while the request side is still open, for example a
request body still in flight. The new code covers that case, and the test now
includes an empty-DATA END_STREAM case as a regression guard.
**Third completion site (`send_headers`).** Fixed. Both
END_STREAM-on-HEADERS signals (EOS for responses and trailers, READ_COMPLETE
for requests) now go through the same `signal_final_read_event()` deferral. You
were right that this was reachable: with the previous revision, a trailers
response hit while throttled stalled until
`transaction_no_activity_timeout_out` fired. The new test reproduces that and
fails against the previous revision.
**Test specificity.** Done. `chunked_flow_control.test.py` now has a custom
HTTP/2 origin (`h2_end_stream_origin.py`). It sends the response header, waits
for ATS to start the tunnel, then writes the body and the end of the stream in
a single write, so END_STREAM reliably lands inside the throttle window. Three
cases end the stream on the last DATA frame, on an empty DATA frame, and on
trailers. The test asserts that `defer …`/`deliver deferred …` are logged for
both READ_COMPLETE and EOS. It also fails if the producer ever sees an
inactivity timeout, and curl gets `--max-time 5`, below the 10s origin timeout.
I moved these cases out of the replay for two reasons. Proxy Verifier can't put
an empty DATA frame directly behind a flow-blocked body (`nghttp2` -529). And
whether END_STREAM landed in the throttle window was timing-dependent there, so
the deferral assertions were flaky in the replay. The replay keeps the 60
KB/512 KB coverage and now pins `proxy-request: protocol: { s
tack: http2 }`.
**Non-blocking notes:**
- Switched to `transaction_no_activity_timeout_out: 10` and pinned
`exec_thread.autoconfig.enabled: 0` / `exec_thread.limit: 1` in the replay.
- `do_io_read` now resets `_deferred_read_event`.
- The `update_read_request` comment is accurate again: `reenable()` no
longer calls it.
- Added a private `_is_read_gated()` predicate used by `is_read_enabled()`,
`update_read_request()`, `signal_read_event()` and `signal_final_read_event()`.
--
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]
