From 1db12de9bed8d4e77f654bb2bcd1c6518b796745 Mon Sep 17 00:00:00 2001 From: Glenn Lewis <6598971+gmlewis@users.noreply.github.com> Date: Fri, 27 Feb 2026 11:09:31 -0500 Subject: [PATCH] fix: Fix data race on Windows Signed-off-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com> --- github/github.go | 21 ++++++++++++++++++++- github/github_test.go | 23 +++++++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/github/github.go b/github/github.go index 9c2cafcced1..53b0c9003d1 100644 --- a/github/github.go +++ b/github/github.go @@ -636,7 +636,20 @@ func (c *Client) NewUploadRequest(urlStr string, reader io.Reader, size int64, m return nil, err } - req, err := http.NewRequest("POST", u.String(), reader) + requestBody := reader + if reader != nil { + // Wrap the provided reader so transport code does not observe concrete body types + // (for example *os.File) and switch to platform-specific sendfile fast paths. + // + // Why this exists: + // race-enabled test runs on Windows have surfaced data races in the sendfile path + // while request read/write loops run concurrently. Hiding concrete type information + // keeps uploads on the generic io.Reader copy path, which is race-stable and preserves + // request semantics (same bytes, same headers, same content length). + requestBody = uploadRequestBodyReader{Reader: reader} + } + + req, err := http.NewRequest("POST", u.String(), requestBody) if err != nil { return nil, err } @@ -658,6 +671,12 @@ func (c *Client) NewUploadRequest(urlStr string, reader io.Reader, size int64, m return req, nil } +// uploadRequestBodyReader intentionally wraps an io.Reader to hide concrete reader types. +// See NewUploadRequest for why this prevents race-prone transport optimizations. +type uploadRequestBodyReader struct { + io.Reader +} + // Response is a GitHub API response. This wraps the standard http.Response // returned from GitHub and provides convenient access to things like // pagination links. diff --git a/github/github_test.go b/github/github_test.go index 034384fe47b..93eb504e384 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "io" + "net" "net/http" "net/http/httptest" "net/url" @@ -31,6 +32,17 @@ const ( baseURLPath = "/api-v3" ) +// raceSafeTestConn wraps a net.Conn to hide concrete connection types such as *net.TCPConn. +// +// Go's HTTP transport may enable OS-level sendfile optimizations when it sees a concrete +// TCP connection and an *os.File request body. Under the race detector on Windows, that +// specific optimized path can trigger a known data race in internal polling structures. +// Returning this wrapper from DialContext keeps behavior identical for tests while forcing +// the transport onto the generic copy path, which is stable under -race. +type raceSafeTestConn struct { + net.Conn +} + // setup sets up a test HTTP server along with a github.Client that is // configured to talk to that test server. Tests should register handlers on // mux which provide mock responses for the API method being tested. @@ -57,8 +69,19 @@ func setup(t *testing.T) (client *Client, mux *http.ServeMux, serverURL string) // server is a test HTTP server used to provide mock API responses. server := httptest.NewServer(apiHandler) + testDialer := &net.Dialer{Timeout: 30 * time.Second} + // Create a custom transport with isolated connection pool transport := &http.Transport{ + // Wrap dialed connections so transport does not take concrete-TCP sendfile fast paths + // that can race under Windows + -race in upload tests. + DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { + conn, err := testDialer.DialContext(ctx, network, addr) + if err != nil { + return nil, err + } + return &raceSafeTestConn{Conn: conn}, nil + }, // Controls connection reuse - false allows reuse, true forces new connections for each request DisableKeepAlives: false, // Maximum concurrent connections per host (active + idle)