bneradt commented on PR #13780: URL: https://github.com/apache/trafficserver/pull/13780#issuecomment-5960588629
Thanks again, @bryancall. I force-pushed an updated commit. Point by point: **Deferral cleared at schedule time.** Fixed. A READ_COMPLETE that `reenable()` schedules now goes back through `signal_final_read_event()` when `main_event_handler` dispatches it. If the consumer throttled again on the same stack, the event is deferred once more and the next `reenable()` reschedules it. I kept clearing the slot when the event is scheduled, so a delivered event can't be replayed by a later `reenable()`. The dispatch-side re-deferral is what keeps it from being lost. `send_tracked_event`'s dedup still covers repeated `reenable()` calls before dispatch. I couldn't reproduce the re-throttle at runtime, and I don't think an HTTP/2 producer can hit it today. The same-stack `disable()` you traced is the plain-passthru ordering at `:1467`, which needs a chunked producer, and HTTP/2 bodies are never chunked on the wire. For `do_chunking` (the H2-origin case), the throttle check at `:1447` runs before the walk, and that synthetic kick only happens after the consumer has dropped below its water mark, so it doesn't trip there. The change is cheap and closes the window regardless, so it's in. **Empty-DATA case and `:187`.** You're right that the GET case never reached it, and I should not have described it as covering it. I tried to reach it in two ways: - Outbound, the response tunnel only starts once the request has been fully sent, so by the time a response END_STREAM can arrive while throttled, the stream is `HALF_CLOSED_LOCAL` and an empty DATA frame takes `initiating_close()`. - Inbound, an HTTP/2 POST without Content-Length that ATS re-chunks to an HTTP/1.1 origin does keep the stream open on ATS's side. But the request body tunnel never throttles, because only the response-side buffers in `HttpSM` get `default_buffer_water_mark` (the post buffers keep `water_mark == 0`, which `_throttle_chunked_producer` skips). I ran that case: chunk mode, no throttle, no deferral. So with today's code, nothing throttles a stream whose request side is still open, and `:187` can't defer. I kept the call there for consistency, so a future water mark on request bodies wouldn't bring the drop back, but no test claims to cover it. The empty-DATA GET case is now described as guarding the `initiating_close()` path, with no deferral assertion. **Per-case assertions.** Each case now runs against its own ATS. The data and trailers cases each assert their own `defer …`/`rescheduled deferred …` lines, so one case can no longer satisfy another's. **Non-blocking notes:** - **Origin flow control:** ATS now uses `server_session_sharing.match: none`, so each request gets a fresh origin connection with full windows. `queue_body` failures print `queue_body_failed` to stdout, and the test excludes that line. - **"deliver deferred" log:** now logged after `send_tracked_event`, worded as "rescheduled deferred …", since delivery itself is shown by the response completing within curl's 5s budget. - **Trailers EOS comment:** updated to say the EOS waits for `reenable()` when throttled. - **Duplicated terminal-event list:** replaced with a shared `_is_final_read_event()`, used by both dispatch paths. - **`BODY_SIZE`:** now documented: under the 65535 windows, and several 16 KB frames past the 4096-byte water mark. - **Header pause:** with one stream per origin connection and a thread per connection, a pause can't hold up another stream. - **Sentinel:** `_deferred_read_event` now uses `VC_EVENT_NONE`. - **Stale TODO:** removed. Locally, all 35 tests in the `h2` and `chunked_encoding` AuTest directories pass, and `chunked_flow_control` passed 3/3 runs. -- 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]
