Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

http: fix connections that are never closed #3647

Closed

Conversation

howardjohn
Copy link

@howardjohn howardjohn commented Apr 24, 2024

This PR addresses an issue we found using hyper with HTTP2 CONNECT. Both the client and server are hyper 1.3.1, with tokio and rustls. In this scenario, we are only sending a single request on the connection.

What we found is that on occasion, despite the Connection object being fully .awaited, the SendRequest dropped, the underlying connection is still open; no GoAway is sent from the client. This hangs forever.

The only way we are able to unblock the connection is if we close it from the server side, or if we use tokio's task dump feature -- this indirectly wakes up every task, allowing things to drive to completion.

Below is my analysis of how this is happening:

Then hyper makes a connection, it spawns a H2ClientFuture, and returns an ClientTask. The user awaits the ClientTask (for us, we spawn it).

The ClientTask future holds a conn_eof: ConnEof. This is a oneshot linked up to H2ClientFuture cancel_tx.

An H2ClientFuture future can finish by either con.poll being ready (i.e. it terminated) or this.drop_rx being ready (i.e. it was dropped). When drop_rx is dropped, we drop cancel_tx. This, in turn, should trigger the ClientTask to drop, and trigger a shutdown -- this is the part that never happens.

drop_rx is half of an mpsc. It was added in the initial HTTP2 support (6 years ago) to detect this exact scenario as far as I can tell. The other end of the mpsc lives in conn_drop_ref in the ClientTask. drop_rx gets Ready((None, Receiver { closed: false })) when the final conn_drop_ref is closed. conn_drop_ref can be cloned in a few places, but for us this never happens (seems like this only happens for not-CONNECT).

Instead, we see ClientTask complete, which drops conn_drop_ref, which sends the message on drop_rx, which triggers us to drop(this.cancel_tx).

This is circular: cancel_tx is supposed to notify conn_eof, which ClientTask is watching.. but we got here because ClientTask dropped!

I got a bit lost there, but I think ultimately this means our ConnTask hangs around forever (we returned Pending but have no way to wake it), which holds onto the h2::Connection which blocks shutdown. The dump-tasks fixing it is from calling poll on ConnTask again, and this time hitting the 'conn terminated' section.


The fix I have applied here is to immediately return ready if drop_rx gets a message. This seems to fix the behavior we see, and I cannot think of cases where this wouldn't be correct, though I only researched the CONNECT flow much

As part of the PR I added a simple regression test. This fails 100% of the time on my machine prior to this PR.

This issue is not caught by existing H2 CONNECT tests which all close the connection from the server side; the test here closes from the client (which, I expect, is more common in the real world?)

@howardjohn howardjohn marked this pull request as ready for review April 25, 2024 17:03
@howardjohn
Copy link
Author

Closing in favor of #3655

@howardjohn howardjohn closed this May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant