upstream: fix review comments

proxy/server_quic.go

@@ -318,7 +318,8 @@ func logShortQUICRead(err error) {
 }
 
 // isQUICNonCrit returns true if err is a non-critical error, most probably
-// a timeout or a closed connection.
+// related to the current QUIC implementation.
+// TODO(ameshkov): re-test when updating quic-go.
 func isQUICNonCrit(err error) (ok bool) {
 	if err == nil {
 		return false

upstream/upstream_doh.go

@@ -214,31 +214,14 @@ func (p *dnsOverHTTPS) shouldRetry(err error) (ok bool) {
 	var netErr net.Error
 	if errors.As(err, &netErr) && netErr.Timeout() {
 		// If this is a timeout error, trying to forcibly re-create the HTTP
-		// client instance.
+		// client instance.  This is an attempt to fix an issue with DoH client
+		// stalling after a network change.
 		//
 		// See https://github.com/AdguardTeam/AdGuardHome/issues/3217.
 		return true
 	}
 
-	var qAppErr *quic.ApplicationError
-	if errors.As(err, &qAppErr) && qAppErr.ErrorCode == 0 {
-		// If this is a QUIC error, we'd better re-create the HTTP client and
-		// reset the token store.  See this example where HTTP3 client does not
-		// handle dead connections:
-		// https://github.com/lucas-clemente/quic-go/issues/765
-		return true
-	}
-
-	var qIdleErr *quic.IdleTimeoutError
-	if errors.As(err, &qIdleErr) {
-		// This error means that the connection was closed due to being idle.
-		// In this case we should forcibly re-create the HTTP client.
-		return true
-	}
-
-	if errors.Is(err, quic.Err0RTTRejected) {
-		// Unfortunately, HTTP3 client does not handle these errors:
-		// https://github.com/lucas-clemente/quic-go/issues/3259
+	if isQUICRetryError(err) {
 		return true
 	}
 
@@ -343,15 +326,6 @@ func (p *dnsOverHTTPS) createTransport() (t http.RoundTripper, err error) {
 	return transport, nil
 }
 
-type http3RoundTripper struct {
-	rt *http3.RoundTripper
-}
-
-func (r *http3RoundTripper) RoundTrip(req *http.Request) (resp *http.Response, err error) {
-	resp, err = r.rt.RoundTrip(req)
-	return resp, err
-}
-
 // createTransportH3 tries to create an HTTP/3 transport for this upstream.
 // We should be able to fall back to H1/H2 in case if HTTP/3 is unavailable or
 // if it is too slow.  In order to do that, this method will run two probes
@@ -388,7 +362,7 @@ func (p *dnsOverHTTPS) createTransportH3(
 		QuicConfig:         p.quicConfig,
 	}
 
-	return &http3RoundTripper{rt: rt}, nil
+	return rt, nil
 }
 
 // probeH3 runs a test to check whether QUIC is faster than TLS for this

upstream/upstream_quic.go

@@ -163,27 +163,7 @@ func (p *dnsOverQUIC) exchangeQUIC(m *dns.Msg) (resp *dns.Msg, err error) {
 // shouldRetry checks what error we received and decides whether it is required
 // to re-open the connection and retry sending the request.
 func (p *dnsOverQUIC) shouldRetry(err error) (ok bool) {
-	var qAppErr *quic.ApplicationError
-	if errors.As(err, &qAppErr) && qAppErr.ErrorCode == 0 {
-		// This error is often returned when the server has been restarted,
-		// and we try to use the same connection.
-		return true
-	}
-
-	var qIdleErr *quic.IdleTimeoutError
-	if errors.As(err, &qIdleErr) {
-		// This error means that the connection was closed due to being idle.
-		// In this case we should forcibly re-create the QUIC connection.
-		return true
-	}
-
-	if errors.Is(err, quic.Err0RTTRejected) {
-		// This error happens when we try to establish a 0-RTT connection with
-		// a token the server is no more aware of.
-		return true
-	}
-
-	return false
+	return isQUICRetryError(err)
 }
 
 // getBytesPool returns (creates if needed) a pool we store byte buffers in.
@@ -358,3 +338,41 @@ func newQUICTokenStore() (s quic.TokenStore) {
 	// more than enough for the way we use it (one connection per upstream).
 	return quic.NewLRUTokenStore(1, 10)
 }
+
+// isQUICRetryError checks the error and determines whether it may signal that
+// we should re-create the QUIC connection.  This requirement is caused by
+// quic-go issues, see the comments inside this function.
+// TODO(ameshkov): re-test when updating quic-go.
+func isQUICRetryError(err error) (ok bool) {
+	var qAppErr *quic.ApplicationError
+	if errors.As(err, &qAppErr) && qAppErr.ErrorCode == 0 {
+		// This error is often returned when the server has been restarted,
+		// and we try to use the same connection on the client-side. It seems,
+		// that the old connections aren't closed immediately on the server-side
+		// and that's why one can run into this.
+		// In addition to that, quic-go HTTP3 client implementation does not
+		// clean up dead connections (this one is specific to DoH3 upstream):
+		// https://github.com/lucas-clemente/quic-go/issues/765
+		return true
+	}
+
+	var qIdleErr *quic.IdleTimeoutError
+	if errors.As(err, &qIdleErr) {
+		// This error means that the connection was closed due to being idle.
+		// In this case we should forcibly re-create the QUIC connection.
+		// Reproducing is rather simple, stop the server and wait for 30 seconds
+		// then try to send another request via the same upstream.
+		return true
+	}
+
+	if errors.Is(err, quic.Err0RTTRejected) {
+		// This error happens when we try to establish a 0-RTT connection with
+		// a token the server is no more aware of.  This can be reproduced by
+		// restarting the QUIC server (it will clear its tokens cache).  The
+		// next connection attempt will return this error until the client's
+		// tokens cache is purged.
+		return true
+	}
+
+	return false
+}