diff --git a/internal/reader/fetcher/request_builder.go b/internal/reader/fetcher/request_builder.go index a067464502a..c7bcb622048 100644 --- a/internal/reader/fetcher/request_builder.go +++ b/internal/reader/fetcher/request_builder.go @@ -38,12 +38,16 @@ type RequestBuilder struct { clientProxyURL *url.URL clientTimeout time.Duration useClientProxy bool + useKeepalive bool withoutRedirects bool ignoreTLSErrors bool disableHTTP2 bool disableCompression bool proxyRotator *proxyrotator.ProxyRotator feedProxyURL string + + // client is lazily built and reused when keepalive is enabled. + client *http.Client } func NewRequestBuilder() *RequestBuilder { @@ -125,6 +129,14 @@ func (r *RequestBuilder) WithoutRedirects() *RequestBuilder { return r } +func (r *RequestBuilder) UseKeepalive(value bool) *RequestBuilder { + r.useKeepalive = value + if !value { + r.Close() + } + return r +} + func (r *RequestBuilder) DisableHTTP2(value bool) *RequestBuilder { r.disableHTTP2 = value return r @@ -141,6 +153,44 @@ func (r *RequestBuilder) WithoutCompression() *RequestBuilder { } func (r *RequestBuilder) ExecuteRequest(requestURL string) (*http.Response, error) { + client, err := r.clientOrNew() + if err != nil { + return nil, err + } + + headers := r.headers.Clone() + if !r.useKeepalive { + headers.Set("Connection", "close") + defer r.Close() + } + + return r.send(client, requestURL, headers) +} + +// Close releases idle connections held by the keep-alive client, if any. +// Safe to call even if no request was sent. +func (r *RequestBuilder) Close() { + if r.client != nil { + r.client.CloseIdleConnections() + r.client = nil + } +} + +func (r *RequestBuilder) clientOrNew() (*http.Client, error) { + if r.client == nil { + client, err := r.newClient() + if err != nil { + return nil, err + } + r.client = client + } + + return r.client, nil +} + +// newClient builds a configured *http.Client from the builder's settings. +// The proxy rotator advances once per call. +func (r *RequestBuilder) newClient() (*http.Client, error) { var clientProxyURL *url.URL switch { @@ -231,14 +281,13 @@ func (r *RequestBuilder) ExecuteRequest(requestURL string) (*http.Response, erro transport.TLSNextProto = map[string]func(string, *tls.Conn) http.RoundTripper{} } - var clientProxyURLRedacted string if clientProxyURL != nil { transport.Proxy = http.ProxyURL(clientProxyURL) - clientProxyURLRedacted = clientProxyURL.Redacted() } client := &http.Client{ - Timeout: r.clientTimeout, + Timeout: r.clientTimeout, + Transport: transport, } if r.withoutRedirects { @@ -247,14 +296,28 @@ func (r *RequestBuilder) ExecuteRequest(requestURL string) (*http.Response, erro } } - client.Transport = transport + var clientProxyURLRedacted string + if clientProxyURL != nil { + clientProxyURLRedacted = clientProxyURL.Redacted() + } + slog.Debug("Built HTTP client", slog.Group("client", + slog.Bool("without_redirects", r.withoutRedirects), + slog.Bool("use_app_client_proxy", r.useClientProxy), + slog.String("client_proxy_url", clientProxyURLRedacted), + slog.Bool("ignore_tls_errors", r.ignoreTLSErrors), + slog.Bool("disable_http2", r.disableHTTP2), + )) + + return client, nil +} +func (r *RequestBuilder) send(client *http.Client, requestURL string, headers http.Header) (*http.Response, error) { req, err := http.NewRequest("GET", requestURL, nil) if err != nil { return nil, err } - req.Header = r.headers + req.Header = headers.Clone() if r.disableCompression { req.Header.Set("Accept-Encoding", "identity") } else { @@ -267,19 +330,11 @@ func (r *RequestBuilder) ExecuteRequest(requestURL string) (*http.Response, erro req.Header.Set("Accept", defaultAcceptHeader) } - req.Header.Set("Connection", "close") - slog.Debug("Making outgoing request", slog.Group("request", slog.String("method", req.Method), slog.String("url", req.URL.String()), slog.Any("headers", req.Header), - slog.Bool("without_redirects", r.withoutRedirects), - slog.Bool("use_app_client_proxy", r.useClientProxy), - slog.String("client_proxy_url", clientProxyURLRedacted), - slog.Bool("ignore_tls_errors", r.ignoreTLSErrors), - slog.Bool("disable_http2", r.disableHTTP2), )) - return client.Do(req) } diff --git a/internal/reader/fetcher/request_builder_test.go b/internal/reader/fetcher/request_builder_test.go index 767f129d9c4..8f0f1b491de 100644 --- a/internal/reader/fetcher/request_builder_test.go +++ b/internal/reader/fetcher/request_builder_test.go @@ -4,10 +4,12 @@ package fetcher // import "miniflux.app/v2/internal/reader/fetcher" import ( + "io" "net/http" "net/http/httptest" "net/url" "strings" + "sync" "testing" "time" @@ -336,6 +338,84 @@ func TestRequestBuilder_ConnectionCloseHeader(t *testing.T) { defer resp.Body.Close() } +func TestRequestBuilder_UseKeepaliveNoConnectionClose(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if got := r.Header.Get("Connection"); got == "close" { + t.Errorf("UseKeepalive should not send 'Connection: close', got %q", got) + } + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + builder := NewRequestBuilder().UseKeepalive(true) + defer builder.Close() + + resp, err := builder.ExecuteRequest(server.URL) + if err != nil { + t.Fatalf("ExecuteRequest: %v", err) + } + resp.Body.Close() +} + +func TestRequestBuilder_UseKeepaliveReusesConnection(t *testing.T) { + var ( + mu sync.Mutex + remotes = map[string]struct{}{} + requests int + ) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + remotes[r.RemoteAddr] = struct{}{} + requests++ + mu.Unlock() + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + builder := NewRequestBuilder().UseKeepalive(true) + defer builder.Close() + + for range 5 { + resp, err := builder.ExecuteRequest(server.URL) + if err != nil { + t.Fatalf("ExecuteRequest: %v", err) + } + // Drain + close so the connection returns to the idle pool and the + // next iteration can reuse it. + io.Copy(io.Discard, resp.Body) + resp.Body.Close() + } + + if requests != 5 { + t.Fatalf("expected 5 requests, got %d", requests) + } + if len(remotes) != 1 { + t.Errorf("expected a single reused TCP connection, got %d distinct remotes: %v", len(remotes), remotes) + } +} + +func TestRequestBuilder_ExecuteRequestDoesNotMutateBuilderHeaders(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + builder := NewRequestBuilder() + resp, err := builder.ExecuteRequest(server.URL) + if err != nil { + t.Fatalf("ExecuteRequest: %v", err) + } + resp.Body.Close() + + // Per-request headers (Accept, Accept-Encoding, Connection) must not leak + // back into the builder so that it can be reused for further requests. + for _, h := range []string{"Accept", "Accept-Encoding", "Connection"} { + if got := builder.headers.Get(h); got != "" { + t.Errorf("builder.headers[%q] leaked from ExecuteRequest: %q", h, got) + } + } +} + func TestRequestBuilder_WithCustomApplicationProxyURL(t *testing.T) { proxyURL, _ := url.Parse("http://proxy.example.com:8080") builder := NewRequestBuilder() diff --git a/internal/reader/subscription/finder.go b/internal/reader/subscription/finder.go index 5244b5b7eb5..3566e5b22fd 100644 --- a/internal/reader/subscription/finder.go +++ b/internal/reader/subscription/finder.go @@ -210,6 +210,13 @@ func (f *subscriptionFinder) findSubscriptionsFromWellKnownURLs(websiteURL strin baseURLs = append(baseURLs, websiteURL) } + // Some websites redirects unknown URLs to the home page. + // As result, the list of known URLs is returned to the subscription list. + // We don't want the user to choose between invalid feed URLs. + f.requestBuilder.WithoutRedirects().UseKeepalive(true) + + defer f.requestBuilder.Close() + var subscriptions Subscriptions for _, baseURL := range baseURLs { for _, known := range knownURLs { @@ -218,12 +225,7 @@ func (f *subscriptionFinder) findSubscriptionsFromWellKnownURLs(websiteURL strin continue } - // Some websites redirects unknown URLs to the home page. - // As result, the list of known URLs is returned to the subscription list. - // We don't want the user to choose between invalid feed URLs. - requestBuilder := f.requestBuilder.WithoutRedirects() - - responseHandler := fetcher.NewResponseHandler(requestBuilder.ExecuteRequest(fullURL)) + responseHandler := fetcher.NewResponseHandler(f.requestBuilder.ExecuteRequest(fullURL)) localizedError := responseHandler.LocalizedError() responseHandler.Close()