Repository: quic-go/quic-go
Pull request #4448 (2024-04-19 13:18:24)

http3: reuse clients on RoundTripOpt context canceled
👋 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.

google-cla[bot]: 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](https://github.com/quic-go/quic-go/pull/4448/checks?check_run_id=24025620534) 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: Thank you @marten-seemann 🙏 
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/4448?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 85.20%. Comparing base [(`e48e1d4`)](https://app.codecov.io/gh/quic-go/quic-go/commit/e48e1d465dffafc1cfd55dc6a4e4642087dcd5eb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) to head [(`7a02178`)](https://app.codecov.io/gh/quic-go/quic-go/pull/4448?dropdown=coverage&src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).


<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #4448      +/-   ##
==========================================
+ Coverage   84.75%   85.20%   +0.46%     
==========================================
  Files         152      149       -3     
  Lines       14371    12835    -1536     
==========================================
- Hits        12179    10936    -1243     
+ Misses       1691     1399     -292     
+ Partials      501      500       -1     
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/quic-go/quic-go/pull/4448?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).


Patch:
diff --git a/http3/roundtrip.go b/http3/roundtrip.go
index 02014c47..f8b0e542 100644
--- a/http3/roundtrip.go
+++ b/http3/roundtrip.go
@@ -165,7 +165,13 @@ func (r *RoundTripper) RoundTripOpt(req *http.Request, opt RoundTripOpt) (*http.
 	defer cl.useCount.Add(-1)
 	rsp, err := cl.rt.RoundTripOpt(req, opt)
 	if err != nil {
-		r.removeClient(hostname)
+		// non-nil errors on roundtrip are likely due to a problem with the connection
+		// so we remove the client from the cache so that subsequent trips reconnect
+		// context cancelation is excluded as is does not signify a connection error
+		if !errors.Is(err, context.Canceled) {
+			r.removeClient(hostname)
+		}
+
 		if isReused {
 			if nerr, ok := err.(net.Error); ok && nerr.Timeout() {
 				return r.RoundTripOpt(req, opt)


Test patch:
diff --git a/http3/roundtrip_test.go b/http3/roundtrip_test.go
index fd8583b9..c314f273 100644
--- a/http3/roundtrip_test.go
+++ b/http3/roundtrip_test.go
@@ -296,6 +296,37 @@ var _ = Describe("RoundTripper", func() {
 			Expect(count).To(Equal(2))
 		})
 
+		It("does not remove a client when a request returns context canceled error", func() {
+			cl1 := NewMockSingleRoundTripper(mockCtrl)
+			clientChan <- cl1
+			cl2 := NewMockSingleRoundTripper(mockCtrl)
+			clientChan <- cl2
+
+			req1, err := http.NewRequest("GET", "https://quic-go.net/foobar.html", nil)
+			Expect(err).ToNot(HaveOccurred())
+			req2, err := http.NewRequest("GET", "https://quic-go.net/bar.html", nil)
+			Expect(err).ToNot(HaveOccurred())
+
+			conn := mockquic.NewMockEarlyConnection(mockCtrl)
+			var count int
+			rt.Dial = func(context.Context, string, *tls.Config, *quic.Config) (quic.EarlyConnection, error) {
+				count++
+				return conn, nil
+			}
+			testErr := context.Canceled
+			handshakeChan := make(chan struct{})
+			close(handshakeChan)
+			conn.EXPECT().HandshakeComplete().Return(handshakeChan).MaxTimes(2)
+			cl1.EXPECT().RoundTripOpt(req1, gomock.Any()).Return(nil, testErr)
+			cl1.EXPECT().RoundTripOpt(req2, gomock.Any()).Return(&http.Response{Request: req2}, nil)
+			_, err = rt.RoundTrip(req1)
+			Expect(err).To(MatchError(testErr))
+			rsp, err := rt.RoundTrip(req2)
+			Expect(err).ToNot(HaveOccurred())
+			Expect(rsp.Request).To(Equal(req2))
+			Expect(count).To(Equal(1))
+		})
+
 		It("recreates a client when a request times out", func() {
 			var reqCount int
 			cl1 := NewMockSingleRoundTripper(mockCtrl)