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.

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

cmd/go: data race in TestScript #54423

Closed
prattmic opened this issue Aug 12, 2022 · 10 comments
Closed

cmd/go: data race in TestScript #54423

prattmic opened this issue Aug 12, 2022 · 10 comments
Assignees
@bcmills
Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Go1.20

Comments

@prattmic
Copy link
Member

From a trybot on https://go.dev/cl/423436: https://storage.googleapis.com/go-build-log/64056b67/linux-amd64-race_b880b74d.log

WARNING: DATA RACE
Read at 0x000001406340 by goroutine 275:
  cmd/go/internal/base.AtExit()
      /workdir/go/src/cmd/go/internal/base/base.go:110 +0x5d0
  cmd/go/internal/work.(*Builder).Init()
      /workdir/go/src/cmd/go/internal/work/action.go:273 +0x54c
  cmd/go_test.(*testScript).cmdCc()
      /workdir/go/src/cmd/go/script_test.go:560 +0x12f
  cmd/go_test.(*testScript).run()
      /workdir/go/src/cmd/go/script_test.go:445 +0xa09
  cmd/go_test.TestScript.func1()
      /workdir/go/src/cmd/go/script_test.go:97 +0x3d5
  testing.tRunner()
      /workdir/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /workdir/go/src/testing/testing.go:1493 +0x47

Previous write at 0x000001406340 by goroutine 274:
  cmd/go/internal/base.AtExit()
      /workdir/go/src/cmd/go/internal/base/base.go:110 +0x678
  cmd/go/internal/work.(*Builder).Init()
      /workdir/go/src/cmd/go/internal/work/action.go:273 +0x54c
  cmd/go_test.(*testScript).cmdCc()
      /workdir/go/src/cmd/go/script_test.go:560 +0x12f
  cmd/go_test.(*testScript).run()
      /workdir/go/src/cmd/go/script_test.go:445 +0xa09
  cmd/go_test.TestScript.func1()
      /workdir/go/src/cmd/go/script_test.go:97 +0x3d5
  testing.tRunner()
      /workdir/go/src/testing/testing.go:1446 +0x216
  testing.(*T).Run.func1()
      /workdir/go/src/testing/testing.go:1493 +0x47

cmd/go/internal/base.AtExit is clearly not thread safe, but I'm not sure why this race is just surfacing. That code and t.Parallel in TestScript both seem to have been there for quite a while.

cc @bcmills
@prattmic prattmic added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 12, 2022
@prattmic prattmic added this to the Backlog milestone Aug 12, 2022
@dmitshur dmitshur added the GoCommand cmd/go label Aug 13, 2022
cuiweixie added a commit to cuiweixie/go that referenced this issue Aug 13, 2022
@cuiweixie
cmd/go: fix data race in TestScript
0b6e832 
Fixes golang#54423
cuiweixie added a commit to cuiweixie/go that referenced this issue Aug 13, 2022
@cuiweixie
cmd/go: fix data race in TestScript
6ed20f7 
Fixes golang#54423
@cuiweixie cuiweixie mentioned this issue Aug 13, 2022
Closed
@gopherbot
Copy link
Contributor

Change https://go.dev/cl/423535 mentions this issue: cmd/go: fix data race in TestScript

cuiweixie added a commit to cuiweixie/go that referenced this issue Aug 14, 2022
@cuiweixie
cmd/go: fix data race in TestScript
13faa5c 
Fixes golang#54423
@bcmills
Copy link
Contributor

bcmills commented Aug 16, 2022

I'm not sure why the race is just now surfacing, but (*testScript).cmdCc should not be registering global hooks in the test process. 😵

Since cmdCc is already explicitly removing b.WorkDir itself, perhaps it should pass some argument to Init to indicate that WorkDir does not need to be cleaned up at exit.

Even better still would be to refactor the cc script command to avoid relying on cmd/go/internal packages directly. Perhaps it could use go env GOGCCFLAGS or similar instead? (It looks like the current approach was added in CL 154109.)

@bcmills bcmills self-assigned this Aug 22, 2022
@bcmills bcmills mentioned this issue Aug 22, 2022
Closed
@bcmills
Copy link
Contributor

bcmills commented Aug 23, 2022

Oh! I do know why the race is just now surfacing.

CL 402596 added a non-short test that uses the cc script command — in addition to the existing one added in CL 186417. We don't have a longtest builder that runs the race detector, so the existing builders didn't cover the racy tests. 😞

I've filed that testing gap as #54630.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/425254 mentions this issue: cmd/go: avoid leaking base.AtExit handlers for Builder instances

@bcmills
Copy link
Contributor

bcmills commented Aug 23, 2022

@gopherbot, please backport to go 1.18 and 1.19. This causes a data race in cmd/go tests.

This was referenced Aug 23, 2022
Closed
Closed
@gopherbot
Copy link
Contributor

Backport issue(s) opened: #54636 (for 1.18), #54637 (for 1.19).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://go.dev/wiki/MinorReleases.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/425206 mentions this issue: cmd/go: skip link_syso tests in short mode

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/425205 mentions this issue: cmd/go: avoid registering AtExit handlers in tests

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/425207 mentions this issue: [release-branch.go1.19] cmd/go: avoid registering AtExit handlers in tests

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/425208 mentions this issue: [release-branch.go1.18] cmd/go: avoid registering AtExit handlers in tests

gopherbot pushed a commit that referenced this issue Aug 24, 2022
@gopherbot
cmd/go/internal/work: make NewBuilder safe for concurrent and repeate…
55d96f9 
…d use

Ever since 'go build' was added (in CL 5483069), it has used an atexit
handler to clean up working directories. At some point (prior to CL
95900044), Init was called multiple times per builder, registering
potentially many atexit handlers that execute asynchronously and make
debugging more difficult.

The use of an AtExit handler also makes the Builder (and anything that
uses it) prone to races: the base.AtExit API is not designed for
concurrent use, but cmd/go is becoming increasingly concurrent over
time. The AtExit handler also makes the Builder inappropriate to use
within a unit-test, since the handlers do not run during the test
function and accumulate over time.

This change makes NewBuilder safe for concurrent use by registering
the AtExit handler only once (during BuildInit, which was already not
safe for concurrent use), and using a sync.Map to store the set of
builders that need cleanup in case of an unclean exit. In addition, it
causes the test variant of cmd/go to fail if any Builder instance
leaks from a clean exit, helping to ensure that functions that create
Builders do not leak them indefinitely, especially in tests.

Updates #54423.

Change-Id: Ia227b15b8fa53c33177c71271d756ac0858feebe
Reviewed-on: https://go-review.googlesource.com/c/go/+/425254
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
gopherbot pushed a commit that referenced this issue Aug 24, 2022
@gopherbot
cmd/go: skip link_syso tests in short mode
2fc21b5 
These tests invoke the system C compiler and linker.
Skipping them saves a little over half a second of time in short mode.

Updates #54423.

Change-Id: I3e8aa7b53c0c91f7d1e001ec2cd5f7b4954de52d
Reviewed-on: https://go-review.googlesource.com/c/go/+/425206
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Than McIntosh <thanm@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
@bcmills bcmills modified the milestones: Backlog, Go1.20 Aug 24, 2022
gopherbot pushed a commit that referenced this issue Aug 29, 2022
@heschi
[release-branch.go1.18] cmd/go: avoid registering AtExit handlers in …
7a575a5 
…tests

Ever since 'go build' was added (in CL 5483069), it has used an atexit
handler to clean up working directories.

CL 154109 introduced 'cc' command to the script test framework that
called Init on a builder once per invocation. Unfortunately, since
base.AtExit is unsynchronized, the Init added there caused any script
that invokes that command to be unsafe for concurrent use.

This change fixes the race by having the 'cc' command pass in its
working directory instead of allowing the Builder to allocate one.
Following modern Go best practices, it also replaces the in-place Init
method (which is prone to typestate and aliasing bugs) with a
NewBuilder constructor function.

Updates #54423.
Fixes #54636.

Change-Id: I8fc2127a7d877bb39a1174e398736bb51d03d4d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/425205
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
(cherry picked from commit d5aa088)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425208
gopherbot pushed a commit that referenced this issue Aug 29, 2022
@heschi
[release-branch.go1.19] cmd/go: avoid registering AtExit handlers in …
4b1c16c 
…tests

Ever since 'go build' was added (in CL 5483069), it has used an atexit
handler to clean up working directories.

CL 154109 introduced 'cc' command to the script test framework that
called Init on a builder once per invocation. Unfortunately, since
base.AtExit is unsynchronized, the Init added there caused any script
that invokes that command to be unsafe for concurrent use.

This change fixes the race by having the 'cc' command pass in its
working directory instead of allowing the Builder to allocate one.
Following modern Go best practices, it also replaces the in-place Init
method (which is prone to typestate and aliasing bugs) with a
NewBuilder constructor function.

Updates #54423.
Fixes #54637.

Change-Id: I8fc2127a7d877bb39a1174e398736bb51d03d4d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/425205
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
(cherry picked from commit d5aa088)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425207
bradfitz pushed a commit to tailscale/go that referenced this issue Sep 8, 2022
@bradfitz
[release-branch.go1.19] cmd/go: avoid registering AtExit handlers in …
af3b81d 
…tests

Ever since 'go build' was added (in CL 5483069), it has used an atexit
handler to clean up working directories.

CL 154109 introduced 'cc' command to the script test framework that
called Init on a builder once per invocation. Unfortunately, since
base.AtExit is unsynchronized, the Init added there caused any script
that invokes that command to be unsafe for concurrent use.

This change fixes the race by having the 'cc' command pass in its
working directory instead of allowing the Builder to allocate one.
Following modern Go best practices, it also replaces the in-place Init
method (which is prone to typestate and aliasing bugs) with a
NewBuilder constructor function.

Updates golang#54423.
Fixes golang#54637.

Change-Id: I8fc2127a7d877bb39a1174e398736bb51d03d4d2
Reviewed-on: https://go-review.googlesource.com/c/go/+/425205
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Bryan Mills <bcmills@google.com>
Auto-Submit: Bryan Mills <bcmills@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>
(cherry picked from commit d5aa088)
Reviewed-on: https://go-review.googlesource.com/c/go/+/425207
@dmitshur dmitshur removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 10, 2022
@dmitshur dmitshur added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsFix The path to resolution is known, but the work has not been done. labels Sep 10, 2022
@bcmills bcmills mentioned this issue Sep 21, 2022
Closed
@golang golang locked and limited conversation to collaborators Sep 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bcmills bcmills

Labels
FrozenDueToAge GoCommand cmd/go NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Go1.20
Development

Successfully merging a pull request may close this issue.

cmd/go: fix data race in TestScript cuiweixie/go
4 participants
@prattmic @dmitshur @bcmills @gopherbot