Skip to content

go/types: data race in Checker.underlying #34921

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
Closed
rsc opened this issue Oct 15, 2019 · 3 comments
Closed

go/types: data race in Checker.underlying #34921

rsc opened this issue Oct 15, 2019 · 3 comments
Labels
FrozenDueToAge
Milestone
Go1.14

Comments

@rsc
Copy link
Contributor

rsc commented Oct 15, 2019

CL 196338, fixing #34333, introduced a data race by caching the result in n.underlying without protecting n with a lock.

Previously, if you had packages A and B both importing C, it was safe to run types.NewChecker(...).Files() on package C's files and then type-check A and B in parallel goroutines both of which returned C's type-check information from their types.Config.Importer.

But now both A and B might end up calling underlying or Underlying on the same type, and the writes updating the new cached value race. For example, here is one trace from #31749:

    WARNING: DATA RACE
    Read at 0x00c0022767f0 by goroutine 200:
      go/types.(*Named).Underlying()
          /workdir/go/src/go/types/type.go:511 +0x3e
      go/types.isTyped()
          /workdir/go/src/go/types/predicates.go:55 +0x83
      go/types.(*Checker).typeDecl()
          /workdir/go/src/go/types/decl.go:544 +0x458
      go/types.(*Checker).objDecl()
          /workdir/go/src/go/types/decl.go:192 +0xcae
      go/types.(*Checker).packageObjects()
          /workdir/go/src/go/types/resolver.go:569 +0x53f
      go/types.(*Checker).checkFiles()
          /workdir/go/src/go/types/check.go:255 +0xd7
      go/types.(*Checker).Files()
          /workdir/go/src/go/types/check.go:246 +0xd3c
      golang.org/x/tools/go/packages.(*loader).loadRecursive.func1()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:685 +0x258
      sync.(*Once).doSlow()
          /workdir/go/src/sync/once.go:66 +0x100
      sync.(*Once).Do()
          /workdir/go/src/sync/once.go:57 +0x68
      golang.org/x/tools/go/packages.(*loader).loadRecursive()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:673 +0x74
      golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:680 +0x42
    
    Previous write at 0x00c0022767f0 by goroutine 339:
      go/types.(*Checker).underlying()
          /workdir/go/src/go/types/decl.go:513 +0x5a6
      go/types.(*Checker).typeDecl()
          /workdir/go/src/go/types/decl.go:559 +0x4ca
      go/types.(*Checker).objDecl()
          /workdir/go/src/go/types/decl.go:192 +0xcae
      go/types.(*Checker).packageObjects()
          /workdir/go/src/go/types/resolver.go:569 +0x53f
      go/types.(*Checker).checkFiles()
          /workdir/go/src/go/types/check.go:255 +0xd7
      go/types.(*Checker).Files()
          /workdir/go/src/go/types/check.go:246 +0xd3c
      golang.org/x/tools/go/packages.(*loader).loadRecursive.func1()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:685 +0x258
      sync.(*Once).doSlow()
          /workdir/go/src/sync/once.go:66 +0x100
      sync.(*Once).Do()
          /workdir/go/src/sync/once.go:57 +0x68
      golang.org/x/tools/go/packages.(*loader).loadRecursive()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:673 +0x74
      golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:680 +0x42
@rsc rsc added this to the Go1.14 milestone Oct 15, 2019
@rsc
Copy link
Contributor Author

rsc commented Oct 15, 2019

The same CL also introduced a similar race in validType. Again from #31749:

    WARNING: DATA RACE
    Read at 0x00c0007ce550 by goroutine 63:
      go/types.(*Checker).validType()
          /workdir/go/src/go/types/decl.go:320 +0x48c
      go/types.(*Checker).validType()
          /workdir/go/src/go/types/decl.go:323 +0x826
      go/types.(*Checker).typeDecl.func1()
          /workdir/go/src/go/types/decl.go:529 +0x8f
      go/types.(*Checker).processDelayed()
          /workdir/go/src/go/types/check.go:281 +0x5a
      go/types.(*Checker).checkFiles()
          /workdir/go/src/go/types/check.go:257 +0xf1
      go/types.(*Checker).Files()
          /workdir/go/src/go/types/check.go:246 +0xd3c
      golang.org/x/tools/go/packages.(*loader).loadRecursive.func1()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:685 +0x258
      sync.(*Once).doSlow()
          /workdir/go/src/sync/once.go:66 +0x100
      sync.(*Once).Do()
          /workdir/go/src/sync/once.go:57 +0x68
      golang.org/x/tools/go/packages.(*loader).loadRecursive()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:673 +0x74
      golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:680 +0x42
    
    Previous write at 0x00c0007ce550 by goroutine 67:
      go/types.(*Checker).validType()
          /workdir/go/src/go/types/decl.go:322 +0x708
      go/types.(*Checker).validType()
          /workdir/go/src/go/types/decl.go:301 +0x249
      go/types.(*Checker).validType()
          /workdir/go/src/go/types/decl.go:323 +0x826
      go/types.(*Checker).typeDecl.func1()
          /workdir/go/src/go/types/decl.go:529 +0x8f
      go/types.(*Checker).processDelayed()
          /workdir/go/src/go/types/check.go:281 +0x5a
      go/types.(*Checker).checkFiles()
          /workdir/go/src/go/types/check.go:257 +0xf1
      go/types.(*Checker).Files()
          /workdir/go/src/go/types/check.go:246 +0xd3c
      golang.org/x/tools/go/packages.(*loader).loadRecursive.func1()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:685 +0x258
      sync.(*Once).doSlow()
          /workdir/go/src/sync/once.go:66 +0x100
      sync.(*Once).Do()
          /workdir/go/src/sync/once.go:57 +0x68
      golang.org/x/tools/go/packages.(*loader).loadRecursive()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:673 +0x74
      golang.org/x/tools/go/packages.(*loader).loadRecursive.func1.1()
          /workdir/gopath/src/golang.org/x/tools/go/packages/packages.go:680 +0x42

@rsc rsc mentioned this issue Oct 15, 2019
Closed
@matloob
Copy link
Contributor

matloob commented Oct 15, 2019

This matches up with what I've been seeing. Before CL 196338 it was okay to concurrently type-check two packages that depend on the same package, which golang.org/x/tools/go/packages does, but not after. I've been running into the second (validType) race consistently.

@griesemer
Copy link
Contributor

Fixed by https://golang.org/cl/201838 (but the CL mentioned #31749 by mistake).

@bcmills bcmills mentioned this issue Oct 21, 2019
Closed
@golang golang locked and limited conversation to collaborators Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.14
Development

No branches or pull requests
4 participants
@rsc @griesemer @gopherbot @matloob