Ok, after a short discussion with Brian Neradt, we've decided that a few tweaks to the current PR https://github.com/apache/trafficserver/pull/11046 should work. I've added a test to make sure the feature (setting "Connection: close" drains the H2 connection) works as intended.
On Tue, Jun 18, 2024 at 12:27 PM Masakazu Kitajo <mas...@apache.org> wrote: > PR 8178 says: > > With this change adding a header-rewrite rule to add the > Connection:closed header will still work to trigger the draining logic. But > core logic to clean up HTTP/1.x connections will not trigger the draining > logic. > > It doesn't sound like the feature was taken out purposefully. I still think > it was broken at some point if it doesn't work, and it should be restored. > When the feature was added on 8.x we didn't have good tools to test it. We > might be able to test it now with ProxyVerifier. Lack of tests should not > be used as justification to silently kill features. > > I do see the complication, but that's inside the core. I don't see how > adding the API, which requires plugins to check HTTP version and do > different things for each version, can be easier for the plugins to use. > The version number in the name should be removed at a minimum (i.e. the API > should work for every HTTP version). > > Masakazu > > > On Mon, Jun 17, 2024 at 8:52 PM Fei Deng <duke8...@gmail.com> wrote: > > > Because of this? > > > https://github.com/apache/trafficserver/pull/11046#discussion_r1583528415 > > > > The code in the HttpTransact.cc that checks for the header removes the > > header and never adds it back if it is a http2 connection. > > > > In an internal discussion in Yahoo I mentioned we can probably find a > exact > > moment to set the header, such that it happens after the check for the > > whole “connection: keep-alive” and “connection: close” in > HttpTransact.cc, > > but at the end it was deemed too complicated, and a simple flag set by an > > api might be easier for the plugins to use. > > > > Do we have a test for that feature mentioned in the docs? > > > > > > Regards, > > Fei Deng > > > > Sent from my iPhone > > > > > > On Mon, Jun 17, 2024 at 10:45 PM Masakazu Kitajo <mas...@apache.org> > > wrote: > > > > > It's a documented feature. > > > > > > > > > https://docs.trafficserver.apache.org/en/9.2.x/admin-guide/plugins/header_rewrite.en.html#close-connections-for-draining > > > > > > > > > https://docs.trafficserver.apache.org/en/latest/admin-guide/plugins/header_rewrite.en.html#close-connections-for-draining > > > > > > On Mon, Jun 17, 2024 at 8:32 PM Masakazu Kitajo <mas...@apache.org> > > wrote: > > > > > > > Purposefully? Where did it happen? I'd say it was broken. We have > that > > > use > > > > case and need a FIX. > > > > > > > > Masakazu > > > > > > > > On Mon, Jun 17, 2024 at 6:47 PM Fei Deng <duke8...@apache.org> > wrote: > > > > > > > >> Actually it’s not possible by setting the “Connection: close” > header. > > > That > > > >> was the initial intention of PR #11046, but with all the discussions > > it > > > >> looks like that functionality was taken out purposefully. > > > >> > > > >> Regards, > > > >> Fei Deng > > > >> > > > >> Sent from my iPhone > > > >> > > > >> > > > >> On Mon, Jun 17, 2024 at 8:42 PM Masakazu Kitajo <mas...@apache.org> > > > >> wrote: > > > >> > > > >> > It has been possible by setting "Connection: Close" header, which > > > means > > > >> > that a server wants no more requests on the connection and wants > to > > > >> close > > > >> > it. > > > >> > > > > >> > If you want to close a connection on some conditions, you could > > check > > > >> the > > > >> > conditions and set the header by header_rewrite (or any plugins). > > And > > > >> those > > > >> > plugins that used to work for H1 do the same for H2 as well > without > > > any > > > >> > changes (and probably for H3 as well, though it's not implemented > > > yet). > > > >> > > > > >> > From plugins' perspective, everything on ATS is HTTP/1. Headers > are > > > >> > converted to H1 representation (e.g. ":authority" -> "Host"). It's > > > >> > natural to use the H1 interface between ATS core and plugins. In > > that > > > >> way, > > > >> > plugins don't even need to know/check the HTTP version. I don't > > think > > > >> > having something just for H2 is a right thing, unless it's truly > an > > H2 > > > >> > specific thing (e.g. setting max H2 frame size). I didn't use "2" > > even > > > >> for > > > >> > ServerPush because I knew H3 was already coming. > > > >> > > > > >> > Masakazu > > > >> > > > > >> > On Mon, Jun 17, 2024 at 9:34 AM Fei Deng <duke8...@apache.org> > > wrote: > > > >> > > > > >> > > TSReturnCode TSHttp2GraceShutdown(TSHttpTxn txnp) > > > >> > > > > > >> > > With this new API, plugins can request a grace shutdown by > sending > > > >> GOAWAY > > > >> > > frames (https://httpwg.org/specs/rfc7540.html#GOAWAY). > > > >> > > > > > >> > > This will also replace this PR > > > >> > > https://github.com/apache/trafficserver/pull/11046 > > > >> > > > > > >> > > Fei Deng > > > >> > > > > > >> > > > > >> > > > > > > > > > >
