Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

net/http: data race in tip after CL 461675 #60041

Open
davecheney opened this issue May 8, 2023 · 4 comments
Open

net/http: data race in tip after CL 461675 #60041

davecheney opened this issue May 8, 2023 · 4 comments
Assignees
@neild
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Milestone
Go1.21

Comments

@davecheney
Copy link
Contributor

What version of Go are you using (go version)?

devel

Does this issue reproduce with the latest release?

Go 1.20.x: no
Tip: yes

What operating system and processor architecture are you using (go env)?

darwin

What did you do?

package main

import (
	"context"
	"errors"
	"net/http"
	"net/http/httptest"
	"strings"
	"testing"
	"time"
)

func TestHTTPRace(t *testing.T) {
	errCh := make(chan error, 1)
	svr := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		t.Logf("request received: %s", r.URL.Path)
		select {
		case <-r.Context().Done():
			errCh <- r.Context().Err()
			return
		case <-time.After(1 * time.Second):
			errCh <- errors.New("request should have been cancelled")
			return
		}
	}))
	svr.EnableHTTP2 = true
	svr.StartTLS()

	ctx, cancel := context.WithCancel(context.Background())
	time.AfterFunc(20*time.Millisecond, func() {
		t.Log("cancelling request")
		cancel()
		t.Log("cancelled request")
	})
	req, err := http.NewRequestWithContext(
		ctx,
		"POST",
		"https://"+svr.Listener.Addr().String()+"/",
		strings.NewReader(`request body triggers the race`),
	)
	if err != nil {
		t.Fatal(err)
	}

	t.Logf("sending request to %s", req.URL)
	_, err = svr.Client().Do(req)
	if !errors.Is(err, context.Canceled) {
		t.Fatalf("expected context cancelled error, got: %v", err)
	}
	if !errors.Is(<-errCh, context.Canceled) {
		t.Fatalf("expected context cancelled error, got: %v", err)
	}
}

% go test -race -count=20 -cpu=1,3,5,7,11

What did you expect to see?

Test pass

What did you see instead?

(~/devel/http-data-race) % go test -race -count=20 -cpu=1,3,5,7,9
==================
WARNING: DATA RACE
Write at 0x00c000010359 by goroutine 24:
  net/http.(*readTrackingBody).Close()
      /Users/davecheney/go/src/net/http/transport.go:657 +0x2f
  net/http.(*http2clientStream).closeReqBodyLocked.func1()
      /Users/davecheney/go/src/net/http/h2_bundle.go:7469 +0x5d

Previous write at 0x00c000010359 by goroutine 6:
  net/http.(*readTrackingBody).Close()
      /Users/davecheney/go/src/net/http/transport.go:657 +0x2f
  net/http.(*Request).closeBody()
      /Users/davecheney/go/src/net/http/request.go:1438 +0xf87
  net/http.(*Transport).roundTrip()
      /Users/davecheney/go/src/net/http/transport.go:629 +0xf48
  net/http.(*Transport).RoundTrip()
      /Users/davecheney/go/src/net/http/roundtrip.go:17 +0x33
  net/http.send()
      /Users/davecheney/go/src/net/http/client.go:260 +0x943
  net/http.(*Client).send()
      /Users/davecheney/go/src/net/http/client.go:181 +0x14e
  net/http.(*Client).do()
      /Users/davecheney/go/src/net/http/client.go:724 +0x1173
  net/http.(*Client).Do()
      /Users/davecheney/go/src/net/http/client.go:590 +0x624
  http-data-race.TestHTTPRace()
      /Users/davecheney/devel/http-data-race/main_test.go:46 +0x60e
  testing.tRunner()
      /Users/davecheney/go/src/testing/testing.go:1595 +0x238
  ??()
      -:0 +0x1177b64

Goroutine 24 (running) created at:
  net/http.(*http2clientStream).closeReqBodyLocked()
      /Users/davecheney/go/src/net/http/h2_bundle.go:7468 +0x170
  net/http.(*http2clientStream).abortStreamLocked()
      /Users/davecheney/go/src/net/http/h2_bundle.go:7443 +0x9e
  net/http.(*http2clientStream).abortStream()
      /Users/davecheney/go/src/net/http/h2_bundle.go:7434 +0xc5
  net/http.(*http2ClientConn).RoundTrip()
      /Users/davecheney/go/src/net/http/h2_bundle.go:8308 +0xb6a
  net/http.(*http2Transport).RoundTripOpt()
      /Users/davecheney/go/src/net/http/h2_bundle.go:7582 +0x624
  net/http.(*http2Transport).RoundTrip()
      /Users/davecheney/go/src/net/http/h2_bundle.go:7534 +0x35
  net/http.(*Transport).roundTrip()
      /Users/davecheney/go/src/net/http/transport.go:602 +0xcee
  net/http.(*Transport).RoundTrip()
      /Users/davecheney/go/src/net/http/roundtrip.go:17 +0x33
  net/http.send()
      /Users/davecheney/go/src/net/http/client.go:260 +0x943
  net/http.(*Client).send()
      /Users/davecheney/go/src/net/http/client.go:181 +0x14e
  net/http.(*Client).do()
      /Users/davecheney/go/src/net/http/client.go:724 +0x1173
  net/http.(*Client).Do()
      /Users/davecheney/go/src/net/http/client.go:590 +0x624
  http-data-race.TestHTTPRace()
      /Users/davecheney/devel/http-data-race/main_test.go:46 +0x60e
  testing.tRunner()
      /Users/davecheney/go/src/testing/testing.go:1595 +0x238
  ??()
      -:0 +0x1177b64

Goroutine 6 (running) created at:
  testing.(*T).Run()
      /Users/davecheney/go/src/testing/testing.go:1648 +0x835
  testing.runTests.func1()
      /Users/davecheney/go/src/testing/testing.go:2054 +0x84
  testing.tRunner()
      /Users/davecheney/go/src/testing/testing.go:1595 +0x238
  testing.runTests()
      /Users/davecheney/go/src/testing/testing.go:2052 +0x896
  testing.(*M).Run()
      /Users/davecheney/go/src/testing/testing.go:1925 +0xb57
  main.main()
      _testmain.go:47 +0x2bd
==================
--- FAIL: TestHTTPRace (0.03s)
    main_test.go:45: sending request to https://127.0.0.1:62439/
    main_test.go:16: request received: /
    main_test.go:31: cancelling request
    main_test.go:33: cancelled request
    testing.go:1465: race detected during execution of test
==================
WARNING: DATA RACE
Read at 0x00c000010551 by goroutine 376:
  net/http.(*Transport).roundTrip()
      /Users/davecheney/go/src/net/http/transport.go:625 +0xf25
  net/http.(*Transport).RoundTrip()
      /Users/davecheney/go/src/net/http/roundtrip.go:17 +0x33
  net/http.send()
      /Users/davecheney/go/src/net/http/client.go:260 +0x943
  net/http.(*Client).send()
      /Users/davecheney/go/src/net/http/client.go:181 +0x14e
  net/http.(*Client).do()
      /Users/davecheney/go/src/net/http/client.go:724 +0x1173
  net/http.(*Client).Do()
      /Users/davecheney/go/src/net/http/client.go:590 +0x624
  http-data-race.TestHTTPRace()
      /Users/davecheney/devel/http-data-race/main_test.go:46 +0x60e
  testing.tRunner()
      /Users/davecheney/go/src/testing/testing.go:1595 +0x238
  ??()
      -:0 +0x1177b64

Previous write at 0x00c000010551 by goroutine 385:
  net/http.(*readTrackingBody).Close()
      /Users/davecheney/go/src/net/http/transport.go:657 +0x2f
  net/http.(*http2clientStream).cleanupWriteRequest()
      /Users/davecheney/go/src/net/http/h2_bundle.go:8545 +0x1d6
  net/http.(*http2clientStream).doRequest()
      /Users/davecheney/go/src/net/http/h2_bundle.go:8322 +0x44
  ??()
      -:0 +0x13ffd24

Goroutine 376 (running) created at:
  testing.(*T).Run()
      /Users/davecheney/go/src/testing/testing.go:1648 +0x835
  testing.runTests.func1()
      /Users/davecheney/go/src/testing/testing.go:2054 +0x84
  testing.tRunner()
      /Users/davecheney/go/src/testing/testing.go:1595 +0x238
  testing.runTests()
      /Users/davecheney/go/src/testing/testing.go:2052 +0x896
  testing.(*M).Run()
      /Users/davecheney/go/src/testing/testing.go:1925 +0xb57
  main.main()
      _testmain.go:47 +0x2bd

Goroutine 385 (running) created at:
  net/http.(*http2ClientConn).RoundTrip()
      /Users/davecheney/go/src/net/http/h2_bundle.go:8250 +0x7ea
  net/http.(*http2Transport).RoundTripOpt()
      /Users/davecheney/go/src/net/http/h2_bundle.go:7582 +0x624
  net/http.(*http2Transport).RoundTrip()
      /Users/davecheney/go/src/net/http/h2_bundle.go:7534 +0x35
  net/http.(*Transport).roundTrip()
      /Users/davecheney/go/src/net/http/transport.go:602 +0xcee
  net/http.(*Transport).RoundTrip()
      /Users/davecheney/go/src/net/http/roundtrip.go:17 +0x33
  net/http.send()
      /Users/davecheney/go/src/net/http/client.go:260 +0x943
  net/http.(*Client).send()
      /Users/davecheney/go/src/net/http/client.go:181 +0x14e
  net/http.(*Client).do()
      /Users/davecheney/go/src/net/http/client.go:724 +0x1173
  net/http.(*Client).Do()
      /Users/davecheney/go/src/net/http/client.go:590 +0x624
  http-data-race.TestHTTPRace()
      /Users/davecheney/devel/http-data-race/main_test.go:46 +0x60e
  testing.tRunner()
      /Users/davecheney/go/src/testing/testing.go:1595 +0x238
  ??()
      -:0 +0x1177b64
==================
--- FAIL: TestHTTPRace (0.03s)
    main_test.go:45: sending request to https://127.0.0.1:62482/
    main_test.go:16: request received: /
    main_test.go:31: cancelling request
    main_test.go:33: cancelled request
    testing.go:1465: race detected during execution of test
^Csignal: interrupt

I believe I have bisected it to this change, #49621, which introduce the type assertion to req.Body.(*readTrackingBody) which elides the read body lock.

/cc @bradfitz @neild
@davecheney davecheney changed the title affected/package: net/http data race in tip after affected/package: net/http data race in tip after https://github.com/golang/go/issues/49621 May 8, 2023
@davecheney davecheney changed the title affected/package: net/http data race in tip after https://github.com/golang/go/issues/49621 net/http data race in tip after https://github.com/golang/go/issues/49621 May 8, 2023
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 8, 2023
@bcmills bcmills changed the title net/http data race in tip after https://github.com/golang/go/issues/49621 net/http data race in tip after CL 461675 May 8, 2023
@bcmills bcmills added this to the Go1.21 milestone May 9, 2023
@dmitshur dmitshur changed the title net/http data race in tip after CL 461675 net/http: data race in tip after CL 461675 May 10, 2023
@bcmills bcmills mentioned this issue May 17, 2023
Open
@heschi
Copy link
Contributor

heschi commented May 17, 2023

@neild do you have an opinion on whether this is a release blocker? Getting very close to the freeze and RC now.

@gopherbot
Copy link

Change https://go.dev/cl/496016 mentions this issue: net/http: avoid race when closing request body

@neild
Copy link
Contributor

neild commented May 17, 2023

Don't know if it's a release blocker, but I believe I have a fix.

@gopherbot
Copy link

Change https://go.dev/cl/496335 mentions this issue: http2: close request bodies before RoundTrip error return

gopherbot pushed a commit to golang/net that referenced this issue May 24, 2023
@neild
http2: close request bodies before RoundTrip error return
6826f5a 
When returning an error from RoundTrip, wait for the close
of the request body to complete before returning.

This avoids a race between the HTTP/2 transport closing
the request body and the net/http retry loop examining
the readTrackingBody to see if it has been closed.

For golang/go#60041

Change-Id: I8be69ff5056806406716e01e02d1f631deeca088
Reviewed-on: https://go-review.googlesource.com/c/net/+/496335
Run-TryBot: Damien Neil <dneil@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@neild neild

Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker
Projects
None yet
Milestone
Go1.21
Development

No branches or pull requests
5 participants
@davecheney @neild @bcmills @gopherbot @heschi