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/tools/imports: TestLocalPrefix is racy #25030

Closed
dsnet opened this issue Apr 23, 2018 · 7 comments
Closed

x/tools/imports: TestLocalPrefix is racy #25030

dsnet opened this issue Apr 23, 2018 · 7 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Unreleased

Comments

@dsnet
Copy link
Member

dsnet commented Apr 23, 2018

Checking out c1def519f03ddf76f16b3e444ee1095d73afa01b and running:

$ go test -race golang.org/x/tools/imports
==================
WARNING: DATA RACE
Write at 0x00c4205dc2f0 by goroutine 35:
  golang.org/x/tools/imports.findImportGoPath()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix.go:849 +0x420
  golang.org/x/tools/imports.fixImports.func2()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix.go:317 +0xc3

Previous write at 0x00c4205dc2f0 by goroutine 39:
  golang.org/x/tools/imports.findImportGoPath()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix.go:849 +0x420
  golang.org/x/tools/imports.fixImports.func2()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix.go:317 +0xc3

Goroutine 35 (running) created at:
  golang.org/x/tools/imports.fixImports()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix.go:304 +0xb13
  golang.org/x/tools/imports.Process()
      /home/rawr/.go/src/golang.org/x/tools/imports/imports.go:66 +0xab5
  golang.org/x/tools/imports.TestLocalPrefix.func1()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix_test.go:1570 +0x21f
  golang.org/x/tools/imports.testConfig.test.func1()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix_test.go:1449 +0x199
  golang.org/x/tools/imports.withEmptyGoPath()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix_test.go:1236 +0x29f
  golang.org/x/tools/imports.testConfig.test()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix_test.go:1437 +0x2df
  golang.org/x/tools/imports.TestLocalPrefix()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix_test.go:1567 +0x45d
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d

Goroutine 39 (running) created at:
  golang.org/x/tools/imports.fixImports()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix.go:304 +0xb13
  golang.org/x/tools/imports.Process()
      /home/rawr/.go/src/golang.org/x/tools/imports/imports.go:66 +0xab5
  golang.org/x/tools/imports.TestLocalPrefix.func1()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix_test.go:1570 +0x21f
  golang.org/x/tools/imports.testConfig.test.func1()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix_test.go:1449 +0x199
  golang.org/x/tools/imports.withEmptyGoPath()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix_test.go:1236 +0x29f
  golang.org/x/tools/imports.testConfig.test()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix_test.go:1437 +0x2df
  golang.org/x/tools/imports.TestLocalPrefix()
      /home/rawr/.go/src/golang.org/x/tools/imports/fix_test.go:1567 +0x45d
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:777 +0x16d
==================
--- FAIL: TestLocalPrefix (0.13s)
	testing.go:730: race detected during execution of test
FAIL
FAIL	golang.org/x/tools/imports	6.488s
@dsnet dsnet added Testing An issue that has been verified to require only test changes, not just a test failure. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Apr 23, 2018
@gopherbot gopherbot added this to the Unreleased milestone Apr 23, 2018
@dsnet
Copy link
Member Author

dsnet commented Apr 23, 2018

git bisect identifies the root cause as https://go-review.googlesource.com/c/tools/+/100460

\cc @nmiyake @bradfitz

@nmiyake
Copy link
Contributor

nmiyake commented Apr 24, 2018

Thanks for flagging.

Given the nature of the change, I highly doubt that the change in fix.go introduced a race (although it may have surfaced it). I'm still investigating locally, but if I leave the change in fix.go but just revert the new test cases that were added, the tests with race detector enabled passes (and conversely, leaving the new test but removing the logic change still shows a race).

@nmiyake
Copy link
Contributor

nmiyake commented Apr 24, 2018

Confirmed that this race is preexisting.

  1. git revert c41d143
  2. Modify line 1502 of fix_test.go to add const Z = foo.X \n:
diff --git a/imports/fix_test.go b/imports/fix_test.go
index dfc3839..a365638 100644
--- a/imports/fix_test.go
+++ b/imports/fix_test.go
@@ -1499,7 +1499,7 @@ func TestLocalPrefix(t *testing.T) {
                                },
                        },
                        localPrefix: "foo/",
-                       src:         "package main \n const Y = bar.X \n const _ = runtime.GOOS",
+                       src:         "package main \n const Y = bar.X \n const Z = foo.X \n const _ = runtime.GOOS",
                        want: `package main
 
 import (

Now, run tests with race:

➜ go test -race golang.org/x/tools/imports
==================
WARNING: DATA RACE
Write at 0x00c4205fd6b0 by goroutine 84:
  golang.org/x/tools/imports.findImportGoPath()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix.go:849 +0x420
  golang.org/x/tools/imports.fixImports.func2()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix.go:317 +0xc3

Previous write at 0x00c4205fd6b0 by goroutine 86:
  golang.org/x/tools/imports.findImportGoPath()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix.go:849 +0x420
  golang.org/x/tools/imports.fixImports.func2()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix.go:317 +0xc3

Goroutine 84 (running) created at:
  golang.org/x/tools/imports.fixImports()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix.go:304 +0xb13
  golang.org/x/tools/imports.Process()
      /Volumes/git/go/src/golang.org/x/tools/imports/imports.go:66 +0xab5
  golang.org/x/tools/imports.TestLocalPrefix.func1()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix_test.go:1547 +0x21f
  golang.org/x/tools/imports.testConfig.test.func1()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix_test.go:1449 +0x199
  golang.org/x/tools/imports.withEmptyGoPath()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix_test.go:1236 +0x29f
  golang.org/x/tools/imports.testConfig.test()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix_test.go:1437 +0x2df
  golang.org/x/tools/imports.TestLocalPrefix()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix_test.go:1544 +0x35f
  testing.tRunner()
      /usr/local/go-1.10.1/src/testing/testing.go:777 +0x16d

Goroutine 86 (running) created at:
  golang.org/x/tools/imports.fixImports()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix.go:304 +0xb13
  golang.org/x/tools/imports.Process()
      /Volumes/git/go/src/golang.org/x/tools/imports/imports.go:66 +0xab5
  golang.org/x/tools/imports.TestLocalPrefix.func1()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix_test.go:1547 +0x21f
  golang.org/x/tools/imports.testConfig.test.func1()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix_test.go:1449 +0x199
  golang.org/x/tools/imports.withEmptyGoPath()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix_test.go:1236 +0x29f
  golang.org/x/tools/imports.testConfig.test()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix_test.go:1437 +0x2df
  golang.org/x/tools/imports.TestLocalPrefix()
      /Volumes/git/go/src/golang.org/x/tools/imports/fix_test.go:1544 +0x35f
  testing.tRunner()
      /usr/local/go-1.10.1/src/testing/testing.go:777 +0x16d
==================
--- FAIL: TestLocalPrefix (0.06s)
        fix_test.go:1552: Got:
                package main
                
                import (
                        "runtime"
                
                        "foo/bar"
                )
                
                const Y = bar.X
                const Z = foo.X
                const _ = runtime.GOOS
                
                Want:
                package main
                
                import (
                        "runtime"
                
                        "foo/bar"
                )
                
                const Y = bar.X
                const _ = runtime.GOOS
        testing.go:730: race detected during execution of test
FAIL
FAIL    golang.org/x/tools/imports      5.479s

@nmiyake
Copy link
Contributor

nmiyake commented Apr 24, 2018 •
edited

The race is occurring here:

// Find candidate packages, looking only at their directory names first.
var candidates []*pkg
for _, pkg := range dirScan {
	if pkgIsCandidate(filename, pkgName, pkg) {
		pkg.distance = distance(pkgDir, pkg.dir)
		candidates = append(candidates, pkg)
	}
}

I believe the issue is that pkg is within the dirScan map, so modifying pkg.distance can race.

This may be naïve, but protecting that assignment using dirScanMu seems to resolve the issue:

// Find candidate packages, looking only at their directory names first.
var candidates []*pkg
for _, pkg := range dirScan {
	if pkgIsCandidate(filename, pkgName, pkg) {
		pkgDistance := distance(pkgDir, pkg.dir)
		dirScanMu.Lock()
		pkg.distance = pkgDistance
		dirScanMu.Unlock()
		candidates = append(candidates, pkg)
	}
}

@bradfitz any thoughts on this?

@bcmills
Copy link
Contributor

bcmills commented Apr 24, 2018

I think the problem is deeper than that: findImportGoPath is computing the distance relative to the current filename and then sorting by that distance. If we happen to run two of those concurrently for files in different paths, we may use the wrong distance.

It's not at all obvious to me whether we run two of those concurrency, but the fact that it's not locally obvious suggests that we shouldn't rely on it either way.

@bcmills
Copy link
Contributor

bcmills commented Apr 24, 2018

I can't resist a good concurrency puzzle. I'll send a fix.

@bcmills bcmills 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 Apr 24, 2018
@bcmills bcmills self-assigned this Apr 24, 2018
@gopherbot
Copy link

Change https://golang.org/cl/109156 mentions this issue: imports: fix races in findImportGoPath

@golang golang locked and limited conversation to collaborators Apr 30, 2019
@rsc rsc unassigned bcmills Jun 23, 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. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@nmiyake @bcmills @dsnet @gopherbot