You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "bneradt (via GitHub)" <gi...@apache.org> on 2026/08/10 14:38:57 UTC
[PR] Shut the client read side down per transaction, not per connection [trafficserver]
bneradt opened a new pull request, #13523:
URL: https://github.com/apache/trafficserver/pull/13523
HttpSM::state_watch_for_client_abort reached past the transaction to
_ua.get_txn()->get_netvc() to half close the client read side on an early
EOS. For HTTP/2 and HTTP/3 that NetVConnection is shared by every stream
on the connection, so a single aborted stream stopped the session from
reading frames for all of the others. Route the shutdown through the
transaction instead: Http2Stream and HQTransaction already implement
do_io_shutdown() as a deliberate no-op for exactly this reason, and
HTTP/1.x is unaffected because ProxyTransaction forwards to the session's
NetVConnection.
This is the remaining half of #12529. That change was written to address
two regressions from #12502, the second being "HTTP/2 connection is
closed if a stream is reset even if other streams are alive", with the
stated approach of calling ProxyTransaction::do_io_shutdown() instead of
NetVConnection::do_io_shutdown(). It converted the two branches it added
but left the pre-existing IO_SHUTDOWN_READ branch calling the
NetVConnection directly, so the connection-wide shutdown survived for the
case where the tunnel still has a consumer besides the client. A response
transform reaches that case readily: the transform stage runs the whole
body before anything is written back, so the stream has no write of its
own and a client reset arrives as an EOS on the stream's read VIO.
Clearing the session's read VIO buffer is not inert, because
Http2CommonSession still holds that VIO and re-enables it every 128
frames through HTTP2_SESSION_EVENT_REENABLE. A release build then takes
the ntodo() <= 0 path in net_read_io and the connection stalls silently,
failing every in-flight stream; a debug build aborts on
SSLNetVConnection's `ink_assert(buf.writer())`, the assertion reported
in #9448.
Co-Authored-By: Claude Opus 5 <no...@anthropic.com>
--
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-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shut the client read side down per transaction, not per connection [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt merged PR #13523:
URL: https://github.com/apache/trafficserver/pull/13523
--
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-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shut the client read side down per transaction, not per connection [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13523:
URL: https://github.com/apache/trafficserver/pull/13523#issuecomment-5243937664
Cherry-picked to the 10.2.x branch as 34a003083beb83a2224b327f67156896b38ab7a9 for the 10.2.0 release.
--
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-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shut the client read side down per transaction, not per connection [trafficserver]
Posted by "bneradt (via GitHub)" <gi...@apache.org>.
bneradt commented on PR #13523:
URL: https://github.com/apache/trafficserver/pull/13523#issuecomment-5242413023
[approve ci fedora freebsd]
--
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-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Shut the client read side down per transaction, not per connection [trafficserver]
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #13523:
URL: https://github.com/apache/trafficserver/pull/13523#issuecomment-5242737076
I traced every claim in the description and they all hold. Recording the verification since the one-line change conceals a fair amount of reasoning.
**The two no-op overrides exist, which is what makes the routing work.** `Http2Stream::do_io_shutdown()` is an inline empty body at `include/proxy/http2/Http2Stream.h:71-74`, and `HQTransaction::do_io_shutdown()` is an explicit `return;` at `Http3Transaction.cc:187`. Worth noting for other reviewers: grepping for `Http2Stream::do_io_shutdown` finds nothing because the H2 one is defined unqualified in the header, which briefly had me thinking the override was missing and the fix a no-op for the very protocol in the title.
**HTTP/1.x is genuinely unaffected.** `ProxyTransaction::do_io_shutdown()` forwards to `_proxy_ssn->do_io_shutdown()` (`ProxyTransaction.cc:219-222`), and `ProxySession::do_io_shutdown()` is `this->_vc->do_io_shutdown(howto)` (`ProxySession.cc:337-340`) — the same NetVConnection the old code reached directly.
**Retaining the `if (netvc)` guard is correct, and for a different reason than before.** `ProxySession::do_io_shutdown()` dereferences `_vc` with no null check, and `ProxyTransaction::get_netvc()` returns `_proxy_ssn ? _proxy_ssn->get_netvc() : nullptr` where `ProxySession::get_netvc()` returns `_vc`. So a non-null `netvc` proves both `_proxy_ssn` and `_proxy_ssn->_vc` are live — exactly the precondition the unconditional dereference needs. The guard is now HTTP/1.x-only in effect, since H2/H3 short-circuit in the transaction before reaching the session, but it is precisely right rather than merely leftover. The comment earns its place.
**The failure mode is exactly as described.** `UnixNetVConnection::do_io_shutdown(IO_SHUTDOWN_READ)` does `read.vio.buffer.clear()`, `read.vio.nbytes = 0`, `read.vio.cont = nullptr`, `read.enabled = 0` — on the *session's* VIO. Then:
- Debug: `SSLNetVConnection::net_read_io()` asserts `ink_assert(buf.writer())` at line 530, before any `ntodo` test. That is #9448.
- Release: the assert compiles out and execution reaches `if (ntodo <= 0 || !buf.writer()->write_avail() || s->vio.is_disabled())` at line 644. Since `nbytes = 0` makes `ntodo() <= 0`, the `||` short-circuits left-to-right *before* `buf.writer()->write_avail()` would dereference the now-null writer, so it takes `read_disable()` and returns. A silent stall rather than a crash — but only by short-circuit ordering, which is worth appreciating.
And the stall is permanent in the way you describe: `Http2CommonSession` re-enables that same VIO every 128 frames (`Http2CommonSession.cc:430`, `HTTP2_SESSION_EVENT_REENABLE`), each time landing back on the `ntodo <= 0` disable. `read.vio.cont = nullptr` means the session has lost its read continuation as well.
**The #12529 history checks out.** Its diff converted the branch it added (`_ua.get_txn()->do_io_shutdown(IO_SHUTDOWN_READWRITE)` for the background-fetch case) and replaced the old `netvc->do_io_shutdown(IO_SHUTDOWN_READWRITE)` with `_ua.get_txn()->do_io_close()`, but the `IO_SHUTDOWN_READ` call inside `if (netvc)` appears as untouched context. So this really is the remaining half.
**The fix is also complete.** `HttpSM.cc:913` is the only site in the tree that reaches past a transaction to a raw NetVConnection for `do_io_shutdown` — every other call in `HttpSM` goes through `p->vc` / `c->vc`, which for the user-agent side *is* the `ProxyTransaction`, so those already hit the no-op override. No sibling instance to chase.
Nice choice using a response transform to reach the case: since the transform stage buffers the whole body before anything is written back, the stream has no write of its own and the client reset arrives as an EOS on the stream's read VIO, which is precisely the `has_consumer_besides_client()` branch. That is much more deterministic than trying to race a reset against a normal proxied write.
**Backport note.** Given the `Crash` label and that this half of #12529 has been latent since it merged, this looks like a 10.2.0 candidate. It should pick cleanly: the new autest's conditions (`HasProxyVerifierVersion('2.8.0')`, `HasOpenSSLVersion`, `PluginExists`) all exist on 10.2.x, that branch pins Proxy Verifier v3.1.3 so the version gate is satisfied, and `null_transform` is present there. Flagging it for the RM rather than assuming.
Nothing blocking from me.
--
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-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org