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

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

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

Comments

@bcmills
Copy link
Member

@bcmills 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

@toothrot
Copy link
Contributor

@toothrot toothrot commented May 26, 2020

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

@stamblerre stamblerre commented May 27, 2020

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
Member Author

@bcmills 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
Member Author

@bcmills 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

@stamblerre stamblerre commented May 27, 2020

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

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 27, 2020

Aha, so it is!

@bcmills
Copy link
Member Author

@bcmills bcmills commented May 27, 2020

Duplicate of #36415

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.