From deb1282f0497b70cd2a6848d21a9442892ee664e Mon Sep 17 00:00:00 2001 From: Rebecca Stambler Date: Thu, 29 Oct 2020 21:39:54 -0400 Subject: [PATCH] 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 Run-TryBot: Rebecca Stambler gopls-CI: kokoro Reviewed-by: Heschi Kreinick --- internal/lsp/cache/load.go | 2 -- internal/lsp/cache/session.go | 40 +++++++++++----------- internal/lsp/cache/snapshot.go | 60 +++++++++++++++++++++----------- internal/lsp/cache/view.go | 62 ++++++++++------------------------ 4 files changed, 78 insertions(+), 86 deletions(-) diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go index 0d07070df0..dd8930c67a 100644 --- a/internal/lsp/cache/load.go +++ b/internal/lsp/cache/load.go @@ -91,8 +91,6 @@ func (s *snapshot) load(ctx context.Context, scopes ...interface{}) error { 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 733c937215..6e34538f87 100644 --- a/internal/lsp/cache/session.go +++ b/internal/lsp/cache/session.go @@ -186,9 +186,6 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, 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 @@ func (s *Session) createView(ctx context.Context, name string, folder span.URI, }, } 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 f26772ec2c..ecd0219051 100644 --- a/internal/lsp/cache/snapshot.go +++ b/internal/lsp/cache/snapshot.go @@ -46,6 +46,33 @@ type snapshot struct { // 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 @@ func (s *snapshot) awaitLoaded(ctx context.Context) error { 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 @@ func (s *snapshot) AwaitInitialized(ctx context.Context) { 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 @@ func generationName(v *View, snapshotID uint64) string { 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 @@ func (s *snapshot) clone(ctx context.Context, withoutURIs map[span.URI]source.Ve 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 @@ copyIDs: // 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 @@ func guessPackagesForURI(uri span.URI, known map[span.URI][]packageID) []package 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 42017a853f..008acefa31 100644 --- a/internal/lsp/cache/view.go +++ b/internal/lsp/cache/view.go @@ -68,33 +68,12 @@ type View struct { 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 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) { 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 @@ func (s *snapshot) initialize(ctx context.Context, firstAttempt bool) { 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 @@ func (v *View) invalidateContent(ctx context.Context, uris map[span.URI]source.V 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 @@ func (v *View) cancelBackground() { 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) {