Skip to content

x/net/http2: data race with TestTransportGroupsPendingDials as of CL 411294 #53480

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.

Sign up for GitHub

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
Closed
bcmills opened this issue Jun 21, 2022 · 1 comment
Closed

x/net/http2: data race with TestTransportGroupsPendingDials as of CL 411294 #53480

bcmills opened this issue Jun 21, 2022 · 1 comment
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations)
Milestone
Go1.19

Comments

@bcmills
Copy link
Contributor

bcmills commented Jun 21, 2022

CL 411294 added an assignment to (and restoration of) the VerboseLogs variable in TestTransportGroupsPendingDials.

Unfortunately, that assignment appears to have introduced a race: it revealed missing synchronization with some other test that apparently leaves behind a goroutine in a call to (*Transport).newClientConn.

==================
WARNING: DATA RACE
Write at 0x000000dfe74d by goroutine 9657:
  golang.org/x/net/http2.TestTransportGroupsPendingDials()
      /workdir/gopath/src/golang.org/x/net/http2/transport_test.go:355 +0xb8
  testing.tRunner()
      /workdir/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /workdir/go/src/testing/testing.go:1493 +0x47

Previous read at 0x000000dfe74d by goroutine 9643:
  golang.org/x/net/http2.(*Transport).vlogf()
      /workdir/gopath/src/golang.org/x/net/http2/transport.go:2897 +0x35e
  golang.org/x/net/http2.(*ClientConn).vlogf()
      /workdir/gopath/src/golang.org/x/net/http2/transport.go:2893 +0x346
  golang.org/x/net/http2.(*clientConnReadLoop).run()
      /workdir/gopath/src/golang.org/x/net/http2/transport.go:2128 +0x269
  golang.org/x/net/http2.(*ClientConn).readLoop()
      /workdir/gopath/src/golang.org/x/net/http2/transport.go:2019 +0x9e
  golang.org/x/net/http2.(*Transport).newClientConn.func1()
      /workdir/gopath/src/golang.org/x/net/http2/transport.go:726 +0x39

Goroutine 9657 (running) created at:
  testing.(*T).Run()
      /workdir/go/src/testing/testing.go:1493 +0x75d
  testing.runTests.func1()
      /workdir/go/src/testing/testing.go:1846 +0x99
  testing.tRunner()
      /workdir/go/src/testing/testing.go:1446 +0x216
  testing.runTests()
      /workdir/go/src/testing/testing.go:1844 +0x7ec
  testing.(*M).Run()
      /workdir/go/src/testing/testing.go:1726 +0xa84
  main.main()
      _testmain.go:741 +0x2e9

Goroutine 9643 (finished) created at:
  golang.org/x/net/http2.(*Transport).newClientConn()
      /workdir/gopath/src/golang.org/x/net/http2/transport.go:726 +0x14eb
  golang.org/x/net/http2.(*Transport).NewClientConn()
      /workdir/gopath/src/golang.org/x/net/http2/transport.go:650 +0xa6
  golang.org/x/net/http2.(*addConnCall).run()
      /workdir/gopath/src/golang.org/x/net/http2/client_conn_pool.go:198 +0x76
  golang.org/x/net/http2.(*clientConnPool).addConnIfNeeded.func1()
      /workdir/gopath/src/golang.org/x/net/http2/client_conn_pool.go:179 +0x71
==================

(attn @neild; CC @bradfitz)
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jun 21, 2022
@bcmills bcmills added this to the Go1.19 milestone Jun 21, 2022
@bcmills bcmills added the Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations) label Jun 21, 2022
@bcmills bcmills mentioned this issue Jun 21, 2022
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/413397 mentions this issue: Revert "http2: enable VerboseLogs in TestTransportGroupsPendingDials"

WeiminShang added a commit to WeiminShang/net that referenced this issue Nov 16, 2022
@WeiminShang @gopherbot
Revert "http2: enable VerboseLogs in TestTransportGroupsPendingDials"
ce5d396 
This reverts CL 411294.

Reason for revert: introduced a data race due to a goroutine left in flight by some other test.

Fixes golang/go#53480.

Change-Id: Ib31b61e7a8eb95dfa585283b7f61bb829916e9dd
Reviewed-on: https://go-review.googlesource.com/c/net/+/413397
Run-TryBot: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
@golang golang locked and limited conversation to collaborators Jun 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Soon This needs action soon. (recent regressions, service outages, unusual time-sensitive situations)
Projects
None yet
Milestone
Go1.19
Development

No branches or pull requests
2 participants
@bcmills @gopherbot