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

x/review/git-codereview: data race in cmdPending via TestBranchConfig #43670

Closed
bcmills opened this issue Jan 13, 2021 · 5 comments
Closed

x/review/git-codereview: data race in cmdPending via TestBranchConfig #43670

bcmills opened this issue Jan 13, 2021 · 5 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone
Go1.16

Comments

@bcmills
Copy link
Contributor

bcmills commented Jan 13, 2021 

==================
WARNING: DATA RACE
Write at 0x000000c6a8c0 by goroutine 15:
  golang.org/x/review/git-codereview.loadGerritOriginInternal()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/api.go:108 +0x7ca
  golang.org/x/review/git-codereview.loadGerritOrigin()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/api.go:51 +0x187
  golang.org/x/review/git-codereview.fullChangeID()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/api.go:285 +0x49
  golang.org/x/review/git-codereview.(*pendingBranch).load()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/pending.go:51 +0x3a5
  golang.org/x/review/git-codereview.cmdPending.func2()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/pending.go:139 +0x50

Previous read at 0x000000c6a8c0 by goroutine 133:
  golang.org/x/review/git-codereview.loadGerritOrigin()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/api.go:42 +0x54
  golang.org/x/review/git-codereview.fullChangeID()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/api.go:285 +0x49
  golang.org/x/review/git-codereview.(*pendingBranch).load()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/pending.go:51 +0x3a5
  golang.org/x/review/git-codereview.cmdPending.func2()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/pending.go:139 +0x50

Goroutine 15 (running) created at:
  golang.org/x/review/git-codereview.cmdPending()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/pending.go:136 +0x51c
  golang.org/x/review/git-codereview.main()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/review.go:141 +0x27e
  golang.org/x/review/git-codereview.testMain()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/util_test.go:359 +0x449
  golang.org/x/review/git-codereview.TestBranchConfig()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/sync_test.go:179 +0xd4b
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:1194 +0x202

Goroutine 133 (running) created at:
  golang.org/x/review/git-codereview.cmdPending()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/pending.go:136 +0x51c
  golang.org/x/review/git-codereview.main()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/review.go:141 +0x27e
  golang.org/x/review/git-codereview.testMain()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/util_test.go:359 +0x449
  golang.org/x/review/git-codereview.TestBranchConfig()
      /tmp/workdir/gopath/src/golang.org/x/review/git-codereview/sync_test.go:179 +0xd4b
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:1194 +0x202
==================

2021-01-13T14:40:17-b8a6970/freebsd-amd64-race
2021-01-13T14:40:11-b916a6f/freebsd-amd64-race
2021-01-13T14:39:58-4aa052d/freebsd-amd64-race

CC @rsc @mdempsky @aclements
@bcmills bcmills added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 13, 2021
@gopherbot gopherbot added this to the Unreleased milestone Jan 13, 2021
@bcmills
Copy link
Contributor Author

bcmills commented Jan 26, 2021 •
edited

I think this is a release-blocker for Go 1.16 via #11811, as it is currently consistently failing on the freebsd-amd64-race builder.
(CC @golang/release)

It isn't clear to me why the race is not being detected on the other -race builders.

@bcmills bcmills modified the milestones: Unreleased, Go1.16 Jan 26, 2021
@mdempsky

This comment has been minimized.

Sign in to view
@dmitshur
Copy link
Contributor

I understand it was meant to be #11811.

If the race is in the x/review code and not a problem in Go 1.16, this likely won't need to block the Go 1.16 release, as no code from x/review is vendored in the main repo.

@bcmills
Copy link
Contributor Author

bcmills commented Jan 26, 2021

Yes, the race is almost certainly in x/review itself.

As I understand it, #11811 is meant to be not only a check on release-regressions caught (or masked) in the x/ repos, but also a backstop on the accumulation of unplanned technical debt in the x/ repos proper.

(I don't see a problem with starting the release with this test still broken, but I also don't think it should just sit on the backlog pile.)

@gopherbot
Copy link

Change https://golang.org/cl/287012 mentions this issue: git-codereview: avoid race in loadGerritOrigin

@dmitshur dmitshur assigned ianlancetaylor and unassigned dmitshur Jan 27, 2021
@dmitshur dmitshur added NeedsFix The path to resolution is known, but the work has not been done. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jan 27, 2021
@golang golang locked and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Milestone
Go1.16
Development

No branches or pull requests
5 participants
@mdempsky @dmitshur @ianlancetaylor @bcmills @gopherbot