bneradt commented on PR #13783: URL: https://github.com/apache/trafficserver/pull/13783#issuecomment-5942449113
Thanks, @bryancall. I force-pushed an updated commit. Point by point: **`set_rx_error_code()` no-op.** Dropped. As you found, the setter only writes `client_info` on inbound streams, so it did nothing here. **No coverage for closing above-boundary streams.** Added two scenarios. - `goaway-split`: three POSTs multiplexed onto one origin connection. The origin sends `GOAWAY(last_stream_id=<lowest>)` and answers only the lowest stream. The test asserts the lowest stream drains with no replay, both higher streams retry on a fresh connection (`retry_admitted 2`), and ATS closes the drained connection itself. This puts two adjacent above-boundary streams through the unlink-safe `while` loop in one pass. With `initiating_close()` removed, this scenario fails. - `goaway-drain-above`: `GOAWAY(last_stream_id=0, NO_ERROR)`, no response, socket held open. The test expects `retry_succeeded attempts=2` and `drained_session_closed_by_peer`. With survivor counting (below), a GOAWAY that leaves nothing to finish falls through to `do_io_close()` as before, so this scenario guards that path rather than the `initiating_close()` itself. The split scenario covers the close. **Our own GOAWAY suppressed.** `_close_connection()` now also sends GOAWAY when the session is draining a peer GOAWAY, so a connection error during the drain still sends one first. **Error-code GOAWAYs.** The drain is now limited to `NO_ERROR`. An error GOAWAY keeps the previous behavior: above-boundary streams are marked retryable and the session closes immediately. **Comments and the survivor count.** The loop now counts surviving streams instead of reading `total_peer_streams_count`. With `last_stream_id=0` it falls through to `do_io_close()` as before, so neither the defer to FINI nor the "Draining" message happens when there is nothing to drain. The debug line now reports survivors and the boundary. I also corrected the `release_stream()` comment (and noted the count can't rise after half-close), replaced "closes it" with "schedules the session close", and marked `_peer_goaway_drain` as outbound-only. The commit message now says the existing transaction timeouts bound the drain. **Test assertions.** `_wait_for_close()` now reports a clean close (EOF, `SSLZeroReturnError`, `SSLEOFError`), a reset (`ConnectionResetError`), or a TLS error, each separately. Only a clean close prints `drained_session_closed_by_peer`. I removed the `ERR_CONNECT_FAIL` exclusion rather than adding more `ERR_*` exclusions, since as you found none of them appear in the sandbox. The verifier/client response checks are what catch the regression. Locally, the whole `h2` autest directory passes, plus a few other tests that use HTTP/2 to origin. -- 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]
