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

cmd/go: data race in TestScript [1.19 backport] #54637

Closed
gopherbot opened this issue Aug 23, 2022 · 3 comments
Closed

cmd/go: data race in TestScript [1.19 backport] #54637

gopherbot opened this issue Aug 23, 2022 · 3 comments
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge GoCommand cmd/go
Milestone

Comments

@gopherbot
Copy link

@bcmills requested issue #54423 to be considered for backport to the next 1.19 minor release.

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

@gopherbot gopherbot added the CherryPickCandidate Used during the release process for point releases label Aug 23, 2022
@gopherbot gopherbot added this to the Go1.19.1 milestone Aug 23, 2022
@bcmills
Copy link
Member

bcmills commented Aug 24, 2022

This is a data race in cmd/go tests, and the fix is low-risk.

@bcmills bcmills added CherryPickApproved Used during the release process for point releases GoCommand cmd/go labels Aug 24, 2022
@gopherbot gopherbot removed the CherryPickCandidate Used during the release process for point releases label Aug 24, 2022
@gopherbot
Copy link
Author

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

@gopherbot
Copy link
Author

Closed by merging 4b1c16c to release-branch.go1.19.

gopherbot pushed a commit that referenced this issue Aug 29, 2022
…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
…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
@golang golang locked and limited conversation to collaborators Aug 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CherryPickApproved Used during the release process for point releases FrozenDueToAge GoCommand cmd/go
Projects
None yet
Development

No branches or pull requests

2 participants