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

http3: reuse clients on RoundTripOpt context canceled #4448

Merged

Conversation

GeorgeMac
Copy link
Contributor

@GeorgeMac GeorgeMac commented Apr 19, 2024

👋 Unsure if this is really a fix or not, but wanted to share for your consideration.

I found that when requests were getting context canceled for whatever reason (e.g. client going away) that the roundtripper was abandoning the connection and dialing again. This PR adds a check to observe the context canceled error and not remove the client in this particular instance. So that the cached client can be used in a subsequent round trip.

I appreciate context canceled can be a bit of a blunt instrument, so I am unsure if this is going to work in the broader state machine, but thought I would open it up for your thoughts.

p.s. love the project. I am attempting to build a reverse tunnel with, which is where I observed this behaviour:
https://github.com/flipt-io/reverst/. In my situation I am proxying a HTTP request from one server onto the http3 client. When e.g. my inbound request is canceled (e.g. the client goes away) this causes the connect to be dropped, which is quite a pain.

Update: I have included this in the project mentioned above and now the connection reuse is much better. I was getting constant reconnections without this change.

Signed-off-by: George MacRorie <me@georgemac.com>
Copy link

google-cla bot commented Apr 19, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@GeorgeMac
Copy link
Contributor Author

I just signed the CLA. Unsure what needs doing to re-kick off that check. Is that something on your end, or does it happen automatically?

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your PR and the thoughtful description.

The change lgtm, we want to remove connections that have already errored, not connections where the application decided to cancel (possibly immediately after dialing).

http3/roundtrip.go Show resolved Hide resolved
@marten-seemann marten-seemann added this to the v0.43 milestone Apr 19, 2024
@marten-seemann marten-seemann changed the title fix(http3): reuse clients on RoundTripOpt context canceled http3: reuse clients on RoundTripOpt context canceled Apr 19, 2024
@GeorgeMac
Copy link
Contributor Author

Thank you @marten-seemann 🙏

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding the explanation!

Copy link

codecov bot commented Apr 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.75%. Comparing base (e48e1d4) to head (7a02178).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4448      +/-   ##
==========================================
+ Coverage   84.75%   84.75%   +0.01%     
==========================================
  Files         152      152              
  Lines       14371    14376       +5     
==========================================
+ Hits        12179    12184       +5     
  Misses       1691     1691              
  Partials      501      501              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marten-seemann marten-seemann merged commit 248189d into quic-go:master Apr 20, 2024
19 checks passed
@GeorgeMac GeorgeMac deleted the gm/reuse-clients-context-cancel branch April 20, 2024 11:24
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

2 participants