diff --git a/tile_fetcher.go b/tile_fetcher.go index 9e45eac..7f31a07 100644 --- a/tile_fetcher.go +++ b/tile_fetcher.go @@ -116,6 +116,7 @@ func (t *TileFetcher) download(url string) ([]byte, error) { if err != nil { return nil, err } + defer resp.Body.Close() switch resp.StatusCode { case http.StatusOK: @@ -128,8 +129,6 @@ func (t *TileFetcher) download(url string) ([]byte, error) { return nil, fmt.Errorf("GET %s: %s", url, resp.Status) } - defer resp.Body.Close() - contents, err := io.ReadAll(resp.Body) if err != nil { return nil, err diff --git a/tile_fetcher_test.go b/tile_fetcher_test.go new file mode 100644 index 0000000..a1e8ad1 --- /dev/null +++ b/tile_fetcher_test.go @@ -0,0 +1,112 @@ +package sm + +import ( + "io" + "net/http" + "net/http/httptest" + "testing" +) + +// closeTrackingBody wraps an io.ReadCloser and records whether Close was called. +type closeTrackingBody struct { + io.ReadCloser + closed bool +} + +func (b *closeTrackingBody) Close() error { + b.closed = true + return b.ReadCloser.Close() +} + +// closeTrackingTransport wraps a Transport and captures the closeTrackingBody +// of the last response it produced, so a test can check whether it was closed. +type closeTrackingTransport struct { + http.RoundTripper + lastBody *closeTrackingBody +} + +func (t *closeTrackingTransport) RoundTrip(req *http.Request) (*http.Response, error) { + resp, err := t.RoundTripper.RoundTrip(req) + if err != nil { + return resp, err + } + wrapped := &closeTrackingBody{ReadCloser: resp.Body} + t.lastBody = wrapped + resp.Body = wrapped + return resp, nil +} + +// withInstrumentedDefaultClient temporarily swaps http.DefaultClient's +// Transport for one that tracks response body closes, and returns a +// restore func plus the tracker itself. +func withInstrumentedDefaultClient(t *testing.T) (*closeTrackingTransport, func()) { + t.Helper() + base := http.DefaultClient.Transport + if base == nil { + base = http.DefaultTransport + } + tracker := &closeTrackingTransport{RoundTripper: base} + orig := http.DefaultClient.Transport + http.DefaultClient.Transport = tracker + return tracker, func() { + http.DefaultClient.Transport = orig + } +} + +// TestTileFetcherDownloadClosesBodyOn404 verifies that TileFetcher.download +// closes the HTTP response body even when the server responds with a +// non-200 status. Leaving it unclosed leaks the underlying connection's +// file descriptor since it is never returned to the transport's pool. +func TestTileFetcherDownloadClosesBodyOn404(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + _, _ = w.Write([]byte("not found")) + })) + defer srv.Close() + + tracker, restore := withInstrumentedDefaultClient(t) + defer restore() + + provider := NewTileProviderOpenStreetMaps() + fetcher := NewTileFetcher(provider, nil, true) + + _, err := fetcher.download(srv.URL) + if err != errTileNotFound { + t.Fatalf("expected errTileNotFound, got %v", err) + } + + if tracker.lastBody == nil { + t.Fatal("no response body was observed") + } + if !tracker.lastBody.closed { + t.Error("response body was not closed on 404 response, leaking the underlying connection") + } +} + +// TestTileFetcherDownloadClosesBodyOnServerError mirrors the 404 case for +// the generic "unexpected status code" error path. +func TestTileFetcherDownloadClosesBodyOnServerError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte("boom")) + })) + defer srv.Close() + + tracker, restore := withInstrumentedDefaultClient(t) + defer restore() + + provider := NewTileProviderOpenStreetMaps() + fetcher := NewTileFetcher(provider, nil, true) + + _, err := fetcher.download(srv.URL) + if err == nil { + t.Fatal("expected an error for 500 response") + } + + if tracker.lastBody == nil { + t.Fatal("no response body was observed") + } + if !tracker.lastBody.closed { + t.Error("response body was not closed on 500 response, leaking the underlying connection") + } +}