internal/lsp: move initialization entirely into the snapshot
The majority of the initialization logic is already based on the
snapshot, not the view, so it makes sense to move it there. The
initialization status of the snapshot is copied and invalidated in
clone.
Fixes golang/go#41764
Change-Id: Icebcf8f4dc750ae4d31e19aa93f9a4c0a57beb4b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/266343
Trust: Rebecca Stambler <rstambler@golang.org>
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
gopls-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Heschi Kreinick <heschi@google.com>
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index 0d07070..dd8930c 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -91,8 +91,6 @@
ctx, done := event.Start(ctx, "cache.view.load", tag.Query.Of(query))
defer done()
- cleanup := func() {}
-
_, inv, cleanup, err := s.goCommandInvocation(ctx, source.ForTypeChecking, &gocommand.Invocation{
WorkingDir: s.view.rootURI.Filename(),
})
diff --git a/internal/lsp/cache/session.go b/internal/lsp/cache/session.go
index 733c937..6e34538 100644
--- a/internal/lsp/cache/session.go
+++ b/internal/lsp/cache/session.go
@@ -186,9 +186,6 @@
v := &View{
session: s,
- initialized: make(chan struct{}),
- initializationSema: make(chan struct{}, 1),
- initializeOnce: &sync.Once{},
id: strconv.FormatInt(index, 10),
options: options,
baseCtx: baseCtx,
@@ -209,23 +206,26 @@
},
}
v.snapshot = &snapshot{
- id: snapshotID,
- view: v,
- generation: s.cache.store.Generation(generationName(v, 0)),
- packages: make(map[packageKey]*packageHandle),
- ids: make(map[span.URI][]packageID),
- metadata: make(map[packageID]*metadata),
- files: make(map[span.URI]source.VersionedFileHandle),
- goFiles: make(map[parseKey]*parseGoHandle),
- importedBy: make(map[packageID][]packageID),
- actions: make(map[actionKey]*actionHandle),
- workspacePackages: make(map[packageID]packagePath),
- unloadableFiles: make(map[span.URI]struct{}),
- parseModHandles: make(map[span.URI]*parseModHandle),
- modTidyHandles: make(map[span.URI]*modTidyHandle),
- modUpgradeHandles: make(map[span.URI]*modUpgradeHandle),
- modWhyHandles: make(map[span.URI]*modWhyHandle),
- modules: modules,
+ id: snapshotID,
+ view: v,
+ initialized: make(chan struct{}),
+ initializationSema: make(chan struct{}, 1),
+ initializeOnce: &sync.Once{},
+ generation: s.cache.store.Generation(generationName(v, 0)),
+ packages: make(map[packageKey]*packageHandle),
+ ids: make(map[span.URI][]packageID),
+ metadata: make(map[packageID]*metadata),
+ files: make(map[span.URI]source.VersionedFileHandle),
+ goFiles: make(map[parseKey]*parseGoHandle),
+ importedBy: make(map[packageID][]packageID),
+ actions: make(map[actionKey]*actionHandle),
+ workspacePackages: make(map[packageID]packagePath),
+ unloadableFiles: make(map[span.URI]struct{}),
+ parseModHandles: make(map[span.URI]*parseModHandle),
+ modTidyHandles: make(map[span.URI]*modTidyHandle),
+ modUpgradeHandles: make(map[span.URI]*modUpgradeHandle),
+ modWhyHandles: make(map[span.URI]*modWhyHandle),
+ modules: modules,
}
v.snapshot.workspaceDirectories = v.snapshot.findWorkspaceDirectories(ctx)
diff --git a/internal/lsp/cache/snapshot.go b/internal/lsp/cache/snapshot.go
index f26772e..ecd0219 100644
--- a/internal/lsp/cache/snapshot.go
+++ b/internal/lsp/cache/snapshot.go
@@ -46,6 +46,33 @@
// builtin pins the AST and package for builtin.go in memory.
builtin *builtinPackageHandle
+ // The snapshot's initialization state is controlled by the fields below.
+ // These fields are propagated across snapshots to avoid multiple
+ // concurrent initializations. They may be invalidated during cloning.
+ //
+ // initialized is closed when the snapshot has been fully initialized. On
+ // initialization, the snapshot's workspace packages are loaded. All of the
+ // fields below are set as part of initialization. If we failed to load, we
+ // only retry if the go.mod file changes, to avoid too many go/packages
+ // calls.
+ //
+ // When the view is created, its snapshot's initializeOnce is non-nil,
+ // initialized is open. Once initialization completes, initializedErr may
+ // be set and initializeOnce becomes nil. If initializedErr is non-nil,
+ // initialization may be retried (depending on how files are changed). To
+ // indicate that initialization should be retried, initializeOnce will be
+ // set. The next time a caller requests workspace packages, the
+ // initialization will retry.
+ initialized chan struct{}
+
+ // initializationSema is used as a mutex to guard initializeOnce and
+ // initializedErr, which will be updated after each attempt to initialize
+ // the snapshot. We use a channel instead of a mutex to avoid blocking when
+ // a context is canceled.
+ initializationSema chan struct{}
+ initializeOnce *sync.Once
+ initializedErr error
+
// mu guards all of the maps in the snapshot.
mu sync.Mutex
@@ -866,7 +893,7 @@
s.mu.Lock()
defer s.mu.Unlock()
if len(s.metadata) == 0 {
- return s.view.initializedErr
+ return s.initializedErr
}
return nil
}
@@ -875,7 +902,7 @@
select {
case <-ctx.Done():
return
- case <-s.view.initialized:
+ case <-s.initialized:
}
// We typically prefer to run something as intensive as the IWL without
// blocking. I'm not sure if there is a way to do that here.
@@ -992,7 +1019,7 @@
return fmt.Sprintf("v%v/%v", v.id, snapshotID)
}
-func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.VersionedFileHandle, forceReloadMetadata bool) (*snapshot, reinitializeView) {
+func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.VersionedFileHandle, forceReloadMetadata bool) *snapshot {
s.mu.Lock()
defer s.mu.Unlock()
@@ -1002,6 +1029,10 @@
generation: newGen,
view: s.view,
builtin: s.builtin,
+ initialized: s.initialized,
+ initializationSema: s.initializationSema,
+ initializeOnce: s.initializeOnce,
+ initializedErr: s.initializedErr,
ids: make(map[span.URI][]packageID),
importedBy: make(map[packageID][]packageID),
metadata: make(map[packageID]*metadata),
@@ -1298,20 +1329,17 @@
// Don't bother copying the importedBy graph,
// as it changes each time we update metadata.
- var reinitialize reinitializeView
- if modulesChanged {
- reinitialize = maybeReinit
- }
- if shouldReinitializeView {
- reinitialize = definitelyReinit
- }
-
// If the snapshot's workspace mode has changed, the packages loaded using
// the previous mode are no longer relevant, so clear them out.
if s.workspaceMode() != result.workspaceMode() {
result.workspacePackages = map[packageID]packagePath{}
}
- return result, reinitialize
+
+ // The snapshot may need to be reinitialized.
+ if modulesChanged || shouldReinitializeView {
+ result.reinitialize(shouldReinitializeView)
+ }
+ return result
}
// guessPackagesForURI returns all packages related to uri. If we haven't seen this
@@ -1364,14 +1392,6 @@
return found
}
-type reinitializeView int
-
-const (
- doNotReinit = reinitializeView(iota)
- maybeReinit
- definitelyReinit
-)
-
// fileWasSaved reports whether the FileHandle passed in has been saved. It
// accomplishes this by checking to see if the original and current FileHandles
// are both overlays, and if the current FileHandle is saved while the original
diff --git a/internal/lsp/cache/view.go b/internal/lsp/cache/view.go
index 42017a8..008acef 100644
--- a/internal/lsp/cache/view.go
+++ b/internal/lsp/cache/view.go
@@ -68,33 +68,12 @@
filesByURI map[span.URI]*fileBase
filesByBase map[string][]*fileBase
- snapshotMu sync.Mutex
- snapshot *snapshot
-
- // initialized is closed when the view has been fully initialized. On
- // initialization, the view's workspace packages are loaded. All of the
- // fields below are set as part of initialization. If we failed to load, we
- // only retry if the go.mod file changes, to avoid too many go/packages
- // calls.
- //
- // When the view is created, initializeOnce is non-nil, initialized is
- // open, and initCancelFirstAttempt can be used to terminate
- // initialization. Once initialization completes, initializedErr may be set
- // and initializeOnce becomes nil. If initializedErr is non-nil,
- // initialization may be retried (depending on how files are changed). To
- // indicate that initialization should be retried, initializeOnce will be
- // set. The next time a caller requests workspace packages, the
- // initialization will retry.
- initialized chan struct{}
+ // initCancelFirstAttempt can be used to terminate the view's first
+ // attempt at initialization.
initCancelFirstAttempt context.CancelFunc
- // initializationSema is used as a mutex to guard initializeOnce and
- // initializedErr, which will be updated after each attempt to initialize
- // the view. We use a channel instead of a mutex to avoid blocking when a
- // context is canceled.
- initializationSema chan struct{}
- initializeOnce *sync.Once
- initializedErr error
+ snapshotMu sync.Mutex
+ snapshot *snapshot
// workspaceInformation tracks various details about this view's
// environment variables, go version, and use of modules.
@@ -495,21 +474,21 @@
select {
case <-ctx.Done():
return
- case s.view.initializationSema <- struct{}{}:
+ case s.initializationSema <- struct{}{}:
}
defer func() {
- <-s.view.initializationSema
+ <-s.initializationSema
}()
- if s.view.initializeOnce == nil {
+ if s.initializeOnce == nil {
return
}
- s.view.initializeOnce.Do(func() {
+ s.initializeOnce.Do(func() {
defer func() {
- s.view.initializeOnce = nil
+ s.initializeOnce = nil
if firstAttempt {
- close(s.view.initialized)
+ close(s.initialized)
}
}()
@@ -557,9 +536,9 @@
if err != nil {
event.Error(ctx, "initial workspace load failed", err)
if modErrors != nil {
- s.view.initializedErr = errors.Errorf("errors loading modules: %v: %w", err, modErrors)
+ s.initializedErr = errors.Errorf("errors loading modules: %v: %w", err, modErrors)
} else {
- s.view.initializedErr = err
+ s.initializedErr = err
}
}
})
@@ -584,14 +563,9 @@
defer v.snapshotMu.Unlock()
oldSnapshot := v.snapshot
- var reinitialize reinitializeView
- v.snapshot, reinitialize = oldSnapshot.clone(ctx, uris, forceReloadMetadata)
+ v.snapshot = oldSnapshot.clone(ctx, uris, forceReloadMetadata)
go oldSnapshot.generation.Destroy()
- if reinitialize == maybeReinit || reinitialize == definitelyReinit {
- v.reinitialize(reinitialize == definitelyReinit)
- }
-
return v.snapshot, v.snapshot.generation.Acquire(ctx)
}
@@ -606,17 +580,17 @@
v.backgroundCtx, v.cancel = context.WithCancel(v.baseCtx)
}
-func (v *View) reinitialize(force bool) {
- v.initializationSema <- struct{}{}
+func (s *snapshot) reinitialize(force bool) {
+ s.initializationSema <- struct{}{}
defer func() {
- <-v.initializationSema
+ <-s.initializationSema
}()
- if !force && v.initializedErr == nil {
+ if !force && s.initializedErr == nil {
return
}
var once sync.Once
- v.initializeOnce = &once
+ s.initializeOnce = &once
}
func (s *Session) getWorkspaceInformation(ctx context.Context, folder span.URI, options *source.Options) (*workspaceInformation, error) {
