refactor: extract shared HTTP timeouts (#1022)

Matt Van Horn and Matt Van Horn created

Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

Change summary

cli/install.go                         |  5 +
internal/httpclient/httpclient.go      | 45 +++++++++++++++
internal/httpclient/httpclient_test.go | 83 ++++++++++++++++++++++++++++
main.go                                | 13 ---
plugin/http.go                         | 12 +--
plugins/embed.go                       |  7 +-
view/html.go                           |  4 
7 files changed, 143 insertions(+), 26 deletions(-)

Detailed changes

cli/install.go 🔗

@@ -7,7 +7,8 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"time"
+
+	"github.com/floatpane/matcha/internal/httpclient"
 )
 
 // RunInstall handles `matcha install <url_or_file>`.
@@ -22,7 +23,7 @@ func RunInstall(args []string) error {
 
 	if strings.HasPrefix(source, "http://") || strings.HasPrefix(source, "https://") {
 		// Download from URL
-		client := &http.Client{Timeout: 30 * time.Second}
+		client := httpclient.New(httpclient.InstallTimeout)
 		resp, err := client.Get(source)
 		if err != nil {
 			return fmt.Errorf("failed to download: %w", err)

internal/httpclient/httpclient.go 🔗

@@ -0,0 +1,45 @@
+// Package httpclient centralizes HTTP timeout defaults so the rest of the
+// codebase doesn't sprinkle magic numbers across packages.
+package httpclient
+
+import (
+	"fmt"
+	"net/http"
+	"time"
+)
+
+// Named timeouts. Each constant documents the call site it covers so
+// future contributors don't have to grep for callers.
+const (
+	// PluginCallTimeout bounds Lua-driven plugin HTTP calls (plugin/http.go).
+	PluginCallTimeout = 10 * time.Second
+	// RegistryFetchTimeout bounds plugin registry / plugin file fetches (plugins/embed.go).
+	RegistryFetchTimeout = 10 * time.Second
+	// RemoteImageTimeout bounds inline image fetches (view/html.go).
+	// Kept short so message rendering doesn't stall.
+	RemoteImageTimeout = 5 * time.Second
+	// InstallTimeout bounds CLI install downloads (cli/install.go).
+	InstallTimeout = 30 * time.Second
+	// UpdateCheckTimeout bounds version checks and asset downloads from main (main.go).
+	UpdateCheckTimeout = 30 * time.Second
+)
+
+// New returns an http.Client preconfigured with the given timeout.
+func New(timeout time.Duration) *http.Client {
+	return &http.Client{Timeout: timeout}
+}
+
+// NewWithRedirectCap returns an http.Client with the given timeout and a
+// hard cap on the number of redirects it will follow before giving up.
+// Used by the main update / asset download client to avoid infinite chains.
+func NewWithRedirectCap(timeout time.Duration, maxRedirects int) *http.Client {
+	return &http.Client{
+		Timeout: timeout,
+		CheckRedirect: func(req *http.Request, via []*http.Request) error {
+			if len(via) >= maxRedirects {
+				return fmt.Errorf("stopped after %d redirects", maxRedirects)
+			}
+			return nil
+		},
+	}
+}

internal/httpclient/httpclient_test.go 🔗

@@ -0,0 +1,83 @@
+package httpclient
+
+import (
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestTimeoutConstants(t *testing.T) {
+	cases := []struct {
+		name string
+		got  time.Duration
+		min  time.Duration
+	}{
+		{"PluginCallTimeout", PluginCallTimeout, time.Second},
+		{"RegistryFetchTimeout", RegistryFetchTimeout, time.Second},
+		{"RemoteImageTimeout", RemoteImageTimeout, time.Second},
+		{"InstallTimeout", InstallTimeout, time.Second},
+		{"UpdateCheckTimeout", UpdateCheckTimeout, time.Second},
+	}
+	for _, c := range cases {
+		if c.got < c.min {
+			t.Errorf("%s = %s, want at least %s", c.name, c.got, c.min)
+		}
+	}
+}
+
+func TestNew_AppliesTimeout(t *testing.T) {
+	c := New(7 * time.Second)
+	if c.Timeout != 7*time.Second {
+		t.Errorf("New(7s).Timeout = %s, want 7s", c.Timeout)
+	}
+}
+
+func TestNewWithRedirectCap_AppliesTimeoutAndRedirects(t *testing.T) {
+	c := NewWithRedirectCap(11*time.Second, 3)
+	if c.Timeout != 11*time.Second {
+		t.Errorf("Timeout = %s, want 11s", c.Timeout)
+	}
+	if c.CheckRedirect == nil {
+		t.Fatal("CheckRedirect is nil; want a redirect-cap function")
+	}
+
+	// Build a stubbed redirect chain and verify the cap fires at the
+	// configured maxRedirects.
+	req, _ := http.NewRequest(http.MethodGet, "http://example.invalid/", nil)
+	via := []*http.Request{}
+	for i := 0; i < 3; i++ {
+		if err := c.CheckRedirect(req, via); err != nil {
+			t.Fatalf("CheckRedirect rejected %d-redirect chain: %v", i, err)
+		}
+		via = append(via, req)
+	}
+	if err := c.CheckRedirect(req, via); err == nil {
+		t.Error("CheckRedirect(via len=3) returned nil; want stopped error")
+	} else if !strings.Contains(err.Error(), "stopped after 3 redirects") {
+		t.Errorf("CheckRedirect error = %q, want 'stopped after 3 redirects' substring", err.Error())
+	}
+}
+
+// TestNewWithRedirectCap_LiveServer is a defense-in-depth integration check
+// that the redirect cap is actually honored by net/http when wired up. It
+// uses an in-process server so it stays hermetic.
+func TestNewWithRedirectCap_LiveServer(t *testing.T) {
+	hops := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		hops++
+		http.Redirect(w, r, "/next", http.StatusFound)
+	}))
+	defer server.Close()
+
+	c := NewWithRedirectCap(2*time.Second, 2)
+	resp, err := c.Get(server.URL + "/start")
+	if err == nil {
+		resp.Body.Close()
+		t.Fatal("expected redirect-cap error, got nil")
+	}
+	if !strings.Contains(err.Error(), "stopped after 2 redirects") {
+		t.Errorf("redirect error = %v, want substring 'stopped after 2 redirects'", err)
+	}
+}

main.go 🔗

@@ -11,7 +11,6 @@ import (
 	"fmt"
 	"io"
 	"log"
-	"net/http"
 	"net/url"
 	"os"
 	"os/exec"
@@ -39,6 +38,7 @@ import (
 	"github.com/floatpane/matcha/fetcher"
 	"github.com/floatpane/matcha/i18n"
 	_ "github.com/floatpane/matcha/i18n/languages"
+	"github.com/floatpane/matcha/internal/httpclient"
 	"github.com/floatpane/matcha/notify"
 	"github.com/floatpane/matcha/plugin"
 	"github.com/floatpane/matcha/sender"
@@ -62,16 +62,7 @@ var (
 	date    = ""
 
 	// httpClient is used for all outbound HTTP requests (update checks, asset downloads).
-	// Configured with a 30s timeout to prevent indefinite hangs on slow/unresponsive servers.
-	httpClient = &http.Client{
-		Timeout: 30 * time.Second,
-		CheckRedirect: func(req *http.Request, via []*http.Request) error {
-			if len(via) >= 5 {
-				return fmt.Errorf("stopped after 5 redirects")
-			}
-			return nil
-		},
-	}
+	httpClient = httpclient.NewWithRedirectCap(httpclient.UpdateCheckTimeout, 5)
 )
 
 // UpdateAvailableMsg is sent into the TUI when a newer release is detected.

plugin/http.go 🔗

@@ -4,19 +4,15 @@ import (
 	"io"
 	"net/http"
 	"strings"
-	"time"
 
 	lua "github.com/yuin/gopher-lua"
-)
 
-const (
-	httpTimeout     = 10 * time.Second
-	httpMaxBodySize = 1 << 20 // 1 MB
+	"github.com/floatpane/matcha/internal/httpclient"
 )
 
-var httpClient = &http.Client{
-	Timeout: httpTimeout,
-}
+const httpMaxBodySize = 1 << 20 // 1 MB
+
+var httpClient = httpclient.New(httpclient.PluginCallTimeout)
 
 // luaHTTP implements matcha.http(options) — make an HTTP request.
 //

plugins/embed.go 🔗

@@ -5,7 +5,8 @@ import (
 	"fmt"
 	"io"
 	"net/http"
-	"time"
+
+	"github.com/floatpane/matcha/internal/httpclient"
 )
 
 const RegistryURL = "https://raw.githubusercontent.com/floatpane/matcha/master/plugins/registry.json"
@@ -22,7 +23,7 @@ type PluginEntry struct {
 
 // FetchRegistry fetches the plugin registry from GitHub.
 func FetchRegistry() ([]PluginEntry, error) {
-	client := &http.Client{Timeout: 10 * time.Second}
+	client := httpclient.New(httpclient.RegistryFetchTimeout)
 	resp, err := client.Get(RegistryURL)
 	if err != nil {
 		return nil, fmt.Errorf("failed to fetch registry: %w", err)
@@ -53,7 +54,7 @@ func FetchPlugin(entry PluginEntry) ([]byte, error) {
 		url = RawPluginBaseURL + entry.File
 	}
 
-	client := &http.Client{Timeout: 10 * time.Second}
+	client := httpclient.New(httpclient.RegistryFetchTimeout)
 	resp, err := client.Get(url)
 	if err != nil {
 		return nil, fmt.Errorf("failed to fetch plugin: %w", err)

view/html.go 🔗

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"io"
 	"mime/quotedprintable"
-	"net/http"
 	"os"
 	"regexp"
 	"strings"
@@ -14,6 +13,7 @@ import (
 
 	"charm.land/lipgloss/v2"
 	"github.com/floatpane/matcha/clib"
+	"github.com/floatpane/matcha/internal/httpclient"
 	"github.com/floatpane/matcha/theme"
 	lru "github.com/hashicorp/golang-lru/v2"
 )
@@ -297,7 +297,7 @@ func fetchRemoteBase64(url string) string {
 		return cached
 	}
 
-	client := &http.Client{Timeout: 5 * time.Second}
+	client := httpclient.New(httpclient.RemoteImageTimeout)
 	resp, err := client.Get(url)
 	if err != nil {
 		debugImageProtocol("remote fetch failed url=%s err=%v", url, err)