Issues with client termination of H2 CONNECT streams

[howardjohn]

Version Hyper 1.3

Platform

6.8.7-arch1-1 #1 SMP PREEMPT_DYNAMIC Wed, 17 Apr 2024 15:20:28 +0000 x86_64 GNU/Linux

Description

We are seeing a few issues around using H2 CONNECT. Our application is using Tokio, Rustls, and Hyper to communicate between a client and server both using the same stack. We have multiple streams on one TCP connection sometimes (though the issue occurs regardless of multiplexing).

Our first issue that popped up was leaking connections. This was debugging and an (attempted) fix was opened in https://github.com/hyperium/hyper/pull/3647. More details there. This was like not detected before because:

• The CONNECT tests in the repo all close from the server side
• When using hyper's pool this isn't seen since the pool will hold onto the connection after all streams are closed (until the idle timeout). We only started seeing this when we stopped using hyper's pool and started closing the connection immediately after the last stream closed.

With that fix, everything was working fine. However, a later refactor in our broke things again; we started dropping the SendRequest before we were complete with IO operations on Upgraded (before, we dropped the SendRequest after). This causes the stream/connection to be closed unexpectedly. This can be reproduced easily by moving the drop(client) in https://github.com/hyperium/hyper/pull/3647 to before the write/read operations. This is easy to workaround, but its not documented and @seanmonstar suggested it was not expected on Discord.

[seanmonstar]

That is surprising. The Connection is design to stay alive until all shared references to it are gone. Those are the SendRequest, and any ResponseFuture, SendStream, and RecvStream. Perhaps something about the way the types are stored inside the Upgraded makes it drop the reference and no longer be counted for liveness?

cc @nox, who worked on this previously.

[howardjohn]

Ah, I think I got things a little mixed up.

In https://github.com/hyperium/hyper/pull/3647, I sent a test and a fix.

• The test was supposed to reproduce "connection is not closed when SendRequest and Upgraded are dropped". It was written incorrectly; I never dropped Upgraded which kept the connection alive as expected.
• My fix closed the connection when all SendRequests dropped, even if there were SendStream, RecvStream, etc left (via Upgraded). So it is the cause of the issue described

That being said, I do still see the issue in our application despite dropping everything, I am just unable to reproduce it in a unit test.

[howardjohn]

Ok I think I figured it out and put up two PRs:

• Test in hyper https://github.com/hyperium/hyper/pull/3655
• Fix in h2 https://github.com/hyperium/h2/pull/772