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

x/tools/go/loader: data race between markErrorFreePackages and (*importer).addFiles #39133

Closed
bcmills opened this issue May 18, 2020 · 7 comments
Closed

x/tools/go/loader: data race between markErrorFreePackages and (*importer).addFiles #39133

bcmills opened this issue May 18, 2020 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
Unreleased

Comments

@bcmills
Copy link
Contributor

bcmills commented May 18, 2020

https://build.golang.org/log/0a21f00ce4288dd398547ef13f5a6abdcd8ed0de

The race involves go/types on both sides, so it's not obvious to me whether this is a bug in the implementation of go/types or in its use within go/loader.

Marking as release-blocker for Go 1.15 in case it is the former.

==================
WARNING: DATA RACE
Read at 0x00c004310c10 by goroutine 54:
  go/types.(*Package).Imports()
      /tmp/workdir/go/src/go/types/package.go:56 +0x2a1
  golang.org/x/tools/go/loader.markErrorFreePackages()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:686 +0x12f
  golang.org/x/tools/go/loader.(*Config).Load()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:669 +0x1d31
  golang.org/x/tools/go/loader_test.TestCycles()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader_test.go:744 +0xac2
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:991 +0x1eb

Previous write at 0x00c004310c10 by goroutine 143:
  go/types.(*Checker).collectObjects()
      /tmp/workdir/go/src/go/types/resolver.go:264 +0x1c8a
  go/types.(*Checker).checkFiles()
      /tmp/workdir/go/src/go/types/check.go:255 +0xd7
  go/types.(*Checker).Files()
      /tmp/workdir/go/src/go/types/check.go:248 +0x2a2
  golang.org/x/tools/go/loader.(*importer).addFiles()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:1030 +0x24c
  golang.org/x/tools/go/loader.(*importer).load()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:989 +0x1f5
  golang.org/x/tools/go/loader.(*importer).startLoad.func1()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:970 +0x46

Goroutine 54 (running) created at:
  testing.(*T).Run()
      /tmp/workdir/go/src/testing/testing.go:1042 +0x660
  testing.runTests.func1()
      /tmp/workdir/go/src/testing/testing.go:1284 +0xa6
  testing.tRunner()
      /tmp/workdir/go/src/testing/testing.go:991 +0x1eb
  testing.runTests()
      /tmp/workdir/go/src/testing/testing.go:1282 +0x527
  testing.(*M).Run()
      /tmp/workdir/go/src/testing/testing.go:1199 +0x2ff
  golang.org/x/tools/go/loader_test.TestMain()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader_test.go:32 +0x3d
  main.main()
      _testmain.go:90 +0x223

Goroutine 143 (finished) created at:
  golang.org/x/tools/go/loader.(*importer).startLoad()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:969 +0x2bb
  golang.org/x/tools/go/loader.(*importer).importAll()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:884 +0x327
  golang.org/x/tools/go/loader.(*importer).addFiles()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:1014 +0x110
  golang.org/x/tools/go/loader.(*importer).load()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:989 +0x1f5
  golang.org/x/tools/go/loader.(*importer).startLoad.func1()
      /tmp/workdir/gopath/src/golang.org/x/tools/go/loader/loader.go:970 +0x46
==================

CC @matloob @stamblerre @griesemer
@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. release-blocker labels May 18, 2020
@bcmills bcmills added this to the Go1.15 milestone May 18, 2020
@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label May 18, 2020
@toothrot
Copy link
Contributor

Hello! This is one of the few remaining issues blocking the Beta release of Go 1.15. We'll need to make a decision on this in the next week in order to keep our release on schedule.

@stamblerre
Copy link
Contributor

I'm really not very familiar with go/loader, but given that it's been relatively unchanged for a while, my inclination is to think the race might be in go/types. From a quick glance at the code, markErrorFreePackages seems to always get called after type-checking is completed and the checker is even explicitly set to nil here. @matloob, what do you think?

@bcmills
Copy link
Contributor Author

bcmills commented May 27, 2020

Given that the failure is detected in TestCycles, I suspect that it is related to this comment:

				// Cycle-forming import: we must not await its
				// completion since it would deadlock.
				//
				// We don't record the error in ii since
				// the error is really associated with the
				// cycle-forming edge, not the package itself.
				// (Also it would complicate the
				// invariants of importPath completion.)

@bcmills
Copy link
Contributor Author

bcmills commented May 27, 2020

(And that code isn't at all new, so this probably doesn't need to be a 1.15 release-blocker. But if it is an old bug, that does raise the question of how it managed to go undetected since ~2015.)

@bcmills bcmills modified the milestones: Go1.15, Unreleased May 27, 2020
@stamblerre
Copy link
Contributor

This also seems to be a duplicate of #36415, so it's been around for a while.

@bcmills
Copy link
Contributor Author

bcmills commented May 27, 2020

Aha, so it is!

@bcmills
Copy link
Contributor Author

bcmills commented May 27, 2020

Duplicate of #36415

@bcmills bcmills marked this as a duplicate of #36415 May 27, 2020
@bcmills bcmills closed this as completed May 27, 2020
@bcmills bcmills mentioned this issue May 27, 2020
Closed
@golang golang locked and limited conversation to collaborators May 27, 2021
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. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@toothrot @bcmills @stamblerre @gopherbot