From 563860d11da684b1d3cbcbe4c57ecf43d099572b Mon Sep 17 00:00:00 2001
From: Heschi Kreinick <heschi@google.com>
Date: Thu, 9 Jan 2020 19:14:18 -0500
Subject: [PATCH] internal/imports: fix use of uninitialized resolvers

Resolvers are lazy initialized. That worked fine until the addition of
the scan semaphore -- it's not a good idea to create that lazily, since
you can't synchronize on a channel that doesn't exist.

Specifically, this caused a gopls hang when completion finished without
needing to use the resolver. In that case, we'd call ClearForNewScan/Mod
on an uninitialized resolver, and then hang receiving from a nil
channel.

Instead, eagerly initialize where convenient, and particularly the scan
semaphore.

Change-Id: I3adb1ae76b751650995e50f50346e06fd9c9f88d
Reviewed-on: https://go-review.googlesource.com/c/tools/+/214258
Run-TryBot: Heschi Kreinick <heschi@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
---
 internal/imports/fix.go      | 31 ++++++++++++++-----------------
 internal/imports/mod.go      | 13 +++++++++----
 internal/imports/mod_test.go |  6 +++---
 3 files changed, 26 insertions(+), 24 deletions(-)

diff --git a/internal/imports/fix.go b/internal/imports/fix.go
index 6a3205b539..8b5d0610b8 100644
--- a/internal/imports/fix.go
+++ b/internal/imports/fix.go
@@ -754,10 +754,10 @@ func (e *ProcessEnv) GetResolver() Resolver {
 	}
 	out, err := e.invokeGo("env", "GOMOD")
 	if err != nil || len(bytes.TrimSpace(out.Bytes())) == 0 {
-		e.resolver = &gopathResolver{env: e}
+		e.resolver = newGopathResolver(e)
 		return e.resolver
 	}
-	e.resolver = &ModuleResolver{env: e}
+	e.resolver = newModuleResolver(e)
 	return e.resolver
 }
 
@@ -998,31 +998,30 @@ type gopathResolver struct {
 	scanSema chan struct{} // scanSema prevents concurrent scans.
 }
 
-func (r *gopathResolver) init() {
-	if r.cache == nil {
-		r.cache = &dirInfoCache{
+func newGopathResolver(env *ProcessEnv) *gopathResolver {
+	r := &gopathResolver{
+		env: env,
+		cache: &dirInfoCache{
 			dirs:      map[string]*directoryPackageInfo{},
 			listeners: map[*int]cacheListener{},
-		}
-	}
-	if r.scanSema == nil {
-		r.scanSema = make(chan struct{}, 1)
-		r.scanSema <- struct{}{}
+		},
+		scanSema: make(chan struct{}, 1),
 	}
+	r.scanSema <- struct{}{}
+	return r
 }
 
 func (r *gopathResolver) ClearForNewScan() {
 	<-r.scanSema
-	*r = gopathResolver{
-		env:      r.env,
-		scanSema: r.scanSema,
+	r.cache = &dirInfoCache{
+		dirs:      map[string]*directoryPackageInfo{},
+		listeners: map[*int]cacheListener{},
 	}
-	r.init()
+	r.walked = false
 	r.scanSema <- struct{}{}
 }
 
 func (r *gopathResolver) loadPackageNames(importPaths []string, srcDir string) (map[string]string, error) {
-	r.init()
 	names := map[string]string{}
 	for _, path := range importPaths {
 		names[path] = importPathToName(r.env, path, srcDir)
@@ -1150,7 +1149,6 @@ func distance(basepath, targetpath string) int {
 }
 
 func (r *gopathResolver) scan(ctx context.Context, callback *scanCallback) error {
-	r.init()
 	add := func(root gopathwalk.Root, dir string) {
 		// We assume cached directories have not changed. We can skip them and their
 		// children.
@@ -1234,7 +1232,6 @@ func filterRoots(roots []gopathwalk.Root, include func(gopathwalk.Root) bool) []
 }
 
 func (r *gopathResolver) loadExports(ctx context.Context, pkg *pkg) (string, []string, error) {
-	r.init()
 	if info, ok := r.cache.Load(pkg.dir); ok {
 		return r.cache.CacheExports(ctx, r.env, info)
 	}
diff --git a/internal/imports/mod.go b/internal/imports/mod.go
index adc6004c2f..d4daf4fd5e 100644
--- a/internal/imports/mod.go
+++ b/internal/imports/mod.go
@@ -49,6 +49,15 @@ type ModuleJSON struct {
 	GoVersion string      // go version used in module
 }
 
+func newModuleResolver(e *ProcessEnv) *ModuleResolver {
+	r := &ModuleResolver{
+		env:      e,
+		scanSema: make(chan struct{}, 1),
+	}
+	r.scanSema <- struct{}{}
+	return r
+}
+
 func (r *ModuleResolver) init() error {
 	if r.Initialized {
 		return nil
@@ -120,10 +129,6 @@ func (r *ModuleResolver) init() error {
 	}
 
 	r.scannedRoots = map[gopathwalk.Root]bool{}
-	if r.scanSema == nil {
-		r.scanSema = make(chan struct{}, 1)
-		r.scanSema <- struct{}{}
-	}
 	if r.moduleCacheCache == nil {
 		r.moduleCacheCache = &dirInfoCache{
 			dirs:      map[string]*directoryPackageInfo{},
diff --git a/internal/imports/mod_test.go b/internal/imports/mod_test.go
index 47d89daea7..fc022ae386 100644
--- a/internal/imports/mod_test.go
+++ b/internal/imports/mod_test.go
@@ -239,7 +239,7 @@ import _ "rsc.io/sampler"
 	}
 
 	// Clear out the resolver's cache, since we've changed the environment.
-	mt.resolver = &ModuleResolver{env: mt.env}
+	mt.resolver = newModuleResolver(mt.env)
 	mt.env.GOFLAGS = "-mod=vendor"
 	mt.assertModuleFoundInDir("rsc.io/sampler", "sampler", `/vendor/`)
 }
@@ -694,7 +694,7 @@ func setup(t *testing.T, main, wd string) *modTest {
 	return &modTest{
 		T:        t,
 		env:      env,
-		resolver: &ModuleResolver{env: env},
+		resolver: newModuleResolver(env),
 		cleanup:  func() { removeDir(dir) },
 	}
 }
@@ -851,7 +851,7 @@ func TestInvalidModCache(t *testing.T) {
 		GOSUMDB:     "off",
 		WorkingDir:  dir,
 	}
-	resolver := &ModuleResolver{env: env}
+	resolver := newModuleResolver(env)
 	scanToSlice(resolver, nil)
 }