internal/lsp: do not close over the handle in the memoize function
Closing over the checkPackageHandle creates a cycle that forces the
checkPackageHandle not to be garbage collected until the value is
created. If a value is never created, the handle will not be collected.
Change-Id: I0f94557da917330ebe307a0e843b16ca7382e210
Reviewed-on: https://go-review.googlesource.com/c/tools/+/204079
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
diff --git a/internal/lsp/cache/check.go b/internal/lsp/cache/check.go
index 3af794f..70416ad 100644
--- a/internal/lsp/cache/check.go
+++ b/internal/lsp/cache/check.go
@@ -33,9 +33,6 @@
// mode is the mode the the files were parsed in.
mode source.ParseMode
- // imports is the map of the package's imports.
- imports map[packagePath]packageID
-
// m is the metadata associated with the package.
m *metadata
@@ -66,24 +63,33 @@
}
// Build the CheckPackageHandle for this ID and its dependencies.
- cph, err := s.buildKey(ctx, id, mode)
+ cph, deps, err := s.buildKey(ctx, id, mode)
if err != nil {
return nil, err
}
- fset := s.View().Session().Cache().FileSet()
- h := s.view.session.cache.store.Bind(string(cph.key), func(ctx context.Context) interface{} {
+
+ // Do not close over the checkPackageHandle or the snapshot in the Bind function.
+ // This creates a cycle, which causes the finalizers to never run on the handles.
+ // The possible cycles are:
+ //
+ // checkPackageHandle.h.function -> checkPackageHandle
+ // checkPackageHandle.h.function -> snapshot -> checkPackageHandle
+ //
+
+ m := cph.m
+ files := cph.files
+ key := cph.key
+ fset := s.view.session.cache.fset
+
+ h := s.view.session.cache.store.Bind(string(key), func(ctx context.Context) interface{} {
// Begin loading the direct dependencies, in parallel.
- for _, impID := range cph.imports {
- dep := s.getPackage(impID, source.ParseExported)
- if dep == nil {
- continue
- }
+ for _, dep := range deps {
go func(dep *checkPackageHandle) {
dep.check(ctx)
}(dep)
}
data := &checkPackageData{}
- data.pkg, data.err = s.typeCheck(ctx, fset, cph)
+ data.pkg, data.err = typeCheck(ctx, fset, m, mode, files, deps)
return data
})
cph.handle = h
@@ -95,31 +101,35 @@
}
// buildKey computes the checkPackageKey for a given checkPackageHandle.
-func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.ParseMode) (*checkPackageHandle, error) {
+func (s *snapshot) buildKey(ctx context.Context, id packageID, mode source.ParseMode) (*checkPackageHandle, map[packagePath]*checkPackageHandle, error) {
m := s.getMetadata(id)
if m == nil {
- return nil, errors.Errorf("no metadata for %s", id)
+ return nil, nil, errors.Errorf("no metadata for %s", id)
}
phs, err := s.parseGoHandles(ctx, m, mode)
if err != nil {
- return nil, err
+ return nil, nil, err
}
cph := &checkPackageHandle{
- m: m,
- files: phs,
- imports: make(map[packagePath]packageID),
- mode: mode,
+ m: m,
+ files: phs,
+ mode: mode,
}
- // Make sure all of the deps are sorted.
- deps := append([]packageID{}, m.deps...)
- sort.Slice(deps, func(i, j int) bool {
- return deps[i] < deps[j]
+ // Make sure all of the depList are sorted.
+ var depList []packageID
+ for _, id := range m.deps {
+ depList = append(depList, id)
+ }
+ sort.Slice(depList, func(i, j int) bool {
+ return depList[i] < depList[j]
})
+ deps := make(map[packagePath]*checkPackageHandle)
+
// Begin computing the key by getting the depKeys for all dependencies.
var depKeys [][]byte
- for _, depID := range deps {
+ for _, depID := range depList {
depHandle, err := s.checkPackageHandle(ctx, depID, source.ParseExported)
if err != nil {
log.Error(ctx, "no dep handle", err, telemetry.Package.Of(depID))
@@ -129,12 +139,11 @@
depKeys = append(depKeys, []byte(fmt.Sprintf("%s import not found", id)))
continue
}
- cph.imports[depHandle.m.pkgPath] = depHandle.m.id
+ deps[depHandle.m.pkgPath] = depHandle
depKeys = append(depKeys, depHandle.key)
}
cph.key = checkPackageKey(cph.m.id, cph.files, m.config, depKeys)
-
- return cph, nil
+ return cph, deps, nil
}
func checkPackageKey(id packageID, phs []source.ParseGoHandle, cfg *packages.Config, deps [][]byte) []byte {
@@ -213,22 +222,22 @@
return phs, nil
}
-func (s *snapshot) typeCheck(ctx context.Context, fset *token.FileSet, cph *checkPackageHandle) (*pkg, error) {
- ctx, done := trace.StartSpan(ctx, "cache.importer.typeCheck", telemetry.Package.Of(cph.m.id))
+func typeCheck(ctx context.Context, fset *token.FileSet, m *metadata, mode source.ParseMode, phs []source.ParseGoHandle, deps map[packagePath]*checkPackageHandle) (*pkg, error) {
+ ctx, done := trace.StartSpan(ctx, "cache.importer.typeCheck", telemetry.Package.Of(m.id))
defer done()
var rawErrors []error
- for _, err := range cph.m.errors {
+ for _, err := range m.errors {
rawErrors = append(rawErrors, err)
}
pkg := &pkg{
- id: cph.m.id,
- mode: cph.mode,
- pkgPath: cph.m.pkgPath,
- files: cph.Files(),
+ id: m.id,
+ pkgPath: m.pkgPath,
+ mode: mode,
+ files: phs,
imports: make(map[packagePath]*pkg),
- typesSizes: cph.m.typesSizes,
+ typesSizes: m.typesSizes,
typesInfo: &types.Info{
Types: make(map[ast.Expr]types.TypeAndValue),
Defs: make(map[*ast.Ident]types.Object),
@@ -274,7 +283,7 @@
} else if len(files) == 0 { // not the unsafe package, no parsed files
return nil, errors.Errorf("no parsed files for package %s", pkg.pkgPath)
} else {
- pkg.types = types.NewPackage(string(cph.m.pkgPath), cph.m.name)
+ pkg.types = types.NewPackage(string(m.pkgPath), m.name)
}
cfg := &types.Config{
@@ -282,13 +291,9 @@
rawErrors = append(rawErrors, e)
},
Importer: importerFunc(func(pkgPath string) (*types.Package, error) {
- impID, ok := cph.imports[packagePath(pkgPath)]
- if !ok {
- return nil, errors.Errorf("no package data for import %s", pkgPath)
- }
- dep := s.getPackage(impID, source.ParseExported)
+ dep := deps[packagePath(pkgPath)]
if dep == nil {
- return nil, errors.Errorf("no package for import %s", impID)
+ return nil, errors.Errorf("no package for import %s", pkgPath)
}
depPkg, err := dep.check(ctx)
if err != nil {
@@ -298,13 +303,13 @@
return depPkg.types, nil
}),
}
- check := types.NewChecker(cfg, s.view.session.cache.FileSet(), pkg.types, pkg.typesInfo)
+ check := types.NewChecker(cfg, fset, pkg.types, pkg.typesInfo)
// Type checking errors are handled via the config, so ignore them here.
_ = check.Files(files)
// We don't care about a package's errors unless we have parsed it in full.
- if cph.mode == source.ParseFull {
+ if mode == source.ParseFull {
for _, e := range rawErrors {
srcErr, err := sourceError(ctx, fset, pkg, e)
if err != nil {
diff --git a/internal/lsp/testdata/bad/badimport.go b/internal/lsp/testdata/bad/badimport.go
index 8960ce1..f7d8a11 100644
--- a/internal/lsp/testdata/bad/badimport.go
+++ b/internal/lsp/testdata/bad/badimport.go
@@ -1,5 +1,5 @@
package bad
import (
- _ "nosuchpkg" //@diag("_", "compiler", "could not import nosuchpkg (no package data for import nosuchpkg)")
+ _ "nosuchpkg" //@diag("_", "compiler", "could not import nosuchpkg (no package for import nosuchpkg)")
)
