Retry idempotent requests once on stale keep-alive connection reuse
httputil.ReverseProxy pools backend connections via sharedTransport but has no built-in retry when it picks a pooled connection the backend has just closed - an inherent HTTP/1.1 keep-alive race. Reproduced this 100% of the time against the LMS backend (lms.noroutes.net) right after its OIDC login callback: the client got a 502 on the very next request, with zero trace of that request in LMS's own logs. LMS's side showed "wthttp/async: handleReadBody(): while waiting for disconnect, received unexpected data, closing" at the same moment - Wt's lightweight built-in httpd closing idle connections more eagerly than Go's pool expects. Retry once, only for idempotent bodyless requests (GET/HEAD/OPTIONS), directly against sharedTransport from within ErrorHandler - standard practice for reverse proxies (nginx's proxy_next_upstream, Traefik's retry middleware). Safe because ReverseProxy hasn't written any response bytes yet when ErrorHandler runs.
This commit is contained in:
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"crypto/tls"
|
||||
"fmt"
|
||||
"io"
|
||||
"log/slog"
|
||||
"net"
|
||||
"net/http"
|
||||
@@ -93,6 +94,35 @@ func buildReverseProxy(rt *runtime.ProxyHostRuntime, backend *loadbalancer.Backe
|
||||
}
|
||||
|
||||
errorHandler := func(w http.ResponseWriter, r *http.Request, err error) {
|
||||
// sharedTransport pools keep-alive connections to the backend. There's
|
||||
// an inherent race in any HTTP/1.1 keep-alive client: the backend can
|
||||
// decide to close an idle connection at (almost) the same moment we
|
||||
// pick it back out of the pool to send the next request, so the
|
||||
// request lands on a connection that's already gone. Go's transport
|
||||
// already retries this automatically in the common case, but not
|
||||
// always (e.g. lightweight backends like Wt's built-in wthttpd, which
|
||||
// can close more eagerly than Go expects - visible in LMS's own logs
|
||||
// as "handleReadBody(): while waiting for disconnect, received
|
||||
// unexpected data, closing"). For idempotent requests with no body,
|
||||
// retrying once here is safe and is standard practice for reverse
|
||||
// proxies (nginx's proxy_next_upstream, Traefik's retry middleware).
|
||||
if isRetryable(r) {
|
||||
log.Warn("backend request failed, retrying once", "proxy_host", rt.Model.Name, "backend", targetAddr, "err", err)
|
||||
retryReq := r.Clone(r.Context())
|
||||
if resp, rerr := sharedTransport.RoundTrip(retryReq); rerr == nil {
|
||||
defer resp.Body.Close()
|
||||
for k, vv := range resp.Header {
|
||||
for _, v := range vv {
|
||||
w.Header().Add(k, v)
|
||||
}
|
||||
}
|
||||
applyCustomHeaders(w.Header(), rt.Model.CustomHeaders, models.HeaderDirectionResponse)
|
||||
w.WriteHeader(resp.StatusCode)
|
||||
io.Copy(w, resp.Body)
|
||||
return
|
||||
}
|
||||
}
|
||||
|
||||
log.Warn("backend request failed", "proxy_host", rt.Model.Name, "backend", targetAddr, "err", err)
|
||||
metrics.HTTPRequestsTotal.WithLabelValues(rt.Model.Name, r.Method, "502").Inc()
|
||||
http.Error(w, "Bad Gateway", http.StatusBadGateway)
|
||||
@@ -107,6 +137,19 @@ func buildReverseProxy(rt *runtime.ProxyHostRuntime, backend *loadbalancer.Backe
|
||||
}
|
||||
}
|
||||
|
||||
// isRetryable reports whether it's safe to silently retry a failed backend
|
||||
// round trip. Only idempotent, bodyless requests qualify: retrying anything
|
||||
// with a request body risks double-submitting it (the body reader may
|
||||
// already be partially or fully consumed by the failed attempt).
|
||||
func isRetryable(r *http.Request) bool {
|
||||
switch r.Method {
|
||||
case http.MethodGet, http.MethodHead, http.MethodOptions:
|
||||
default:
|
||||
return false
|
||||
}
|
||||
return r.Body == nil || r.Body == http.NoBody || r.ContentLength == 0
|
||||
}
|
||||
|
||||
func applyCustomHeaders(h http.Header, headers []models.CustomHeader, direction models.CustomHeaderDirection) {
|
||||
for _, c := range headers {
|
||||
if c.Direction != direction {
|
||||
|
||||
Reference in New Issue
Block a user