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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
