diff --git a/github/github.go b/github/github.go index 07b08ea27bf..e72fd1474c1 100644 --- a/github/github.go +++ b/github/github.go @@ -797,6 +797,30 @@ func WithVersion(version string) RequestOption { } } +// requestBufferPool pools response read buffers so that Client.Do can decode +// JSON payloads without allocating a growing buffer per call. +var requestBufferPool = sync.Pool{ + New: func() any { + return new(bytes.Buffer) + }, +} + +// maxPooledBufferCap is the largest buffer capacity that putRequestBuffer +// returns to requestBufferPool. Buffers that grew beyond it while reading an +// unusually large response are dropped so they do not pin memory in the pool. +const maxPooledBufferCap = 1 << 20 + +// putRequestBuffer resets buf and returns it to requestBufferPool, dropping +// oversized buffers. It reports whether buf was pooled. +func putRequestBuffer(buf *bytes.Buffer) bool { + if buf.Cap() > maxPooledBufferCap { + return false + } + buf.Reset() + requestBufferPool.Put(buf) + return true +} + // NewRequest creates an API request. A relative URL can be provided in urlStr, // in which case it is resolved relative to the BaseURL of the Client. // Relative URLs should always be specified without a preceding slash. If @@ -1409,12 +1433,21 @@ func (c *Client) Do(req *http.Request, v any) (*Response, error) { case io.Writer: _, err = io.Copy(v, resp.Body) default: - decErr := json.NewDecoder(resp.Body).Decode(v) - if decErr == io.EOF { - decErr = nil // ignore EOF errors caused by empty response body - } - if decErr != nil { - err = decErr + respBuf := requestBufferPool.Get().(*bytes.Buffer) + defer putRequestBuffer(respBuf) + + _, readErr := respBuf.ReadFrom(resp.Body) + if readErr != nil { + err = readErr + } else if respBuf.Len() > 0 { + b := respBuf.Bytes() + decErr := json.Unmarshal(b, v) + if decErr != nil && len(bytes.TrimSpace(b)) == 0 { + decErr = nil // ignore errors caused by whitespace-only response body + } + if decErr != nil { + err = decErr + } } } return resp, err diff --git a/github/github_benchmark_test.go b/github/github_benchmark_test.go new file mode 100644 index 00000000000..47a61fe7239 --- /dev/null +++ b/github/github_benchmark_test.go @@ -0,0 +1,60 @@ +// Copyright 2026 The go-github AUTHORS. All rights reserved. +// +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package github + +import ( + "bytes" + "encoding/json" + "fmt" + "io" + "net/http" + "strings" + "testing" +) + +// fixedResponseTransport serves a canned JSON payload without touching the +// network so that BenchmarkDo measures the real request/decode path alone. +type fixedResponseTransport struct { + payload []byte +} + +func (t *fixedResponseTransport) RoundTrip(req *http.Request) (*http.Response, error) { + return &http.Response{ + StatusCode: http.StatusOK, + Header: make(http.Header), + Body: io.NopCloser(bytes.NewReader(t.payload)), + Request: req, + }, nil +} + +func BenchmarkDo(b *testing.B) { + for _, sizeKB := range []int{1, 500} { + b.Run(fmt.Sprintf("%vKB", sizeKB), func(b *testing.B) { + payload, err := json.Marshal(map[string]string{"body": strings.Repeat("a", sizeKB*1024)}) + if err != nil { + b.Fatalf("json.Marshal returned error: %v", err) + } + client, err := NewClient(WithHTTPClient(&http.Client{ + Transport: &fixedResponseTransport{payload: payload}, + })) + if err != nil { + b.Fatalf("NewClient returned error: %v", err) + } + req, err := client.NewRequest(b.Context(), "GET", ".", nil) + if err != nil { + b.Fatalf("NewRequest returned error: %v", err) + } + + b.ReportAllocs() + for b.Loop() { + var v map[string]string + if _, err := client.Do(req, &v); err != nil { + b.Fatalf("Do returned error: %v", err) + } + } + }) + } +} diff --git a/github/github_test.go b/github/github_test.go index ae585cede4d..671b18ec8a9 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -3248,6 +3248,182 @@ func TestDo_noContent(t *testing.T) { } } +func TestDo_ioWriter(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, "hello world") + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + var buf bytes.Buffer + _, err := client.Do(req, &buf) + assertNilError(t, err) + + if buf.String() != "hello world" { + t.Errorf("Response body = %q, want %q", buf.String(), "hello world") + } +} + +func TestDo_ioWriter_error(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, "hello world") + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + var w writerThatErrors + _, err := client.Do(req, &w) + if err == nil { + t.Fatal("Expected error from io.Writer, got none") + } +} + +type writerThatErrors struct{} + +func (w *writerThatErrors) Write(_ []byte) (int, error) { + return 0, errors.New("write error") +} + +func TestDo_invalidJSON(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + type foo struct { + A string + } + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, "not valid json") + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + var body foo + _, err := client.Do(req, &body) + if err == nil { + t.Fatal("Expected JSON decode error, got none") + } +} + +func TestDo_whitespaceOnlyBody(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + type foo struct { + A string + } + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, " \n\t ") + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + var body foo + _, err := client.Do(req, &body) + assertNilError(t, err) + + if body.A != "" { + t.Errorf("Response body = %v, want empty foo", body) + } +} + +func TestDo_nilV_noop(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, `{"A":"a"}`) + }) + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + resp, err := client.Do(req, nil) + assertNilError(t, err) + if resp.StatusCode != http.StatusOK { + t.Errorf("Expected status %v, got %v", http.StatusOK, resp.StatusCode) + } +} + +func TestDo_readError(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + // Announce more bytes than are sent so that reading the response + // body fails with an unexpected EOF. + w.Header().Set("Content-Length", "64") + fmt.Fprint(w, `{"A":"a"}`) + }) + + type foo struct { + A string + } + + req, _ := client.NewRequest(t.Context(), "GET", ".", nil) + var body foo + _, err := client.Do(req, &body) + if err == nil { + t.Fatal("Expected body read error, got none") + } +} + +func TestDo_largeThenSmallBody(t *testing.T) { + t.Parallel() + client, mux, _ := setup(t) + + large := strings.Repeat("a", 2*maxPooledBufferCap) + mux.HandleFunc("/large", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprintf(w, `{"A":%q}`, large) + }) + mux.HandleFunc("/small", func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "GET") + fmt.Fprint(w, `{"A":"small"}`) + }) + + type foo struct { + A string + } + + req, _ := client.NewRequest(t.Context(), "GET", "large", nil) + var body foo + _, err := client.Do(req, &body) + assertNilError(t, err) + if body.A != large { + t.Error("Large response body was not decoded correctly") + } + + req, _ = client.NewRequest(t.Context(), "GET", "small", nil) + body = foo{} + _, err = client.Do(req, &body) + assertNilError(t, err) + if body.A != "small" { + t.Errorf("Response body = %v, want %v", body.A, "small") + } +} + +func TestPutRequestBuffer(t *testing.T) { + t.Parallel() + + if !putRequestBuffer(new(bytes.Buffer)) { + t.Error("putRequestBuffer returned false for a small buffer, want true") + } + + oversized := new(bytes.Buffer) + oversized.Grow(maxPooledBufferCap + 1) + if putRequestBuffer(oversized) { + t.Error("putRequestBuffer returned true for an oversized buffer, want false") + } +} + func TestClient_checkRequestAPIVersionBeforeDo(t *testing.T) { t.Parallel()