[gopls-release-branch.0.9] internal/lsp/cache: clear shouldLoad IDs on load
CL 417576 externalized shouldLoad tracking into a map, which was used to
trigger a reload and cleared once reload completes. Unfortunately, it
overlooked the fact that we may also reload the entire workspace (via
reinitialization). In this case, we should clear newly loaded IDs from
the shouldLoad map, so that they are not subsequently loaded again.
For golang/go#54842
Updates golang/go#54473
Change-Id: I26f49552cae502644142dc4a4e946294db37f6f7
Reviewed-on: https://go-review.googlesource.com/c/tools/+/424074
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
(cherry picked from commit e55fb40e67ba08fdaebb2e477544eb560f198210)
Reviewed-on: https://go-review.googlesource.com/c/tools/+/428596
Reviewed-by: Hyang-Ah Hana Kim <hyangah@gmail.com>
diff --git a/gopls/internal/regtest/workspace/workspace_test.go b/gopls/internal/regtest/workspace/workspace_test.go
index 86da9d1..f474dbf 100644
--- a/gopls/internal/regtest/workspace/workspace_test.go
+++ b/gopls/internal/regtest/workspace/workspace_test.go
@@ -156,6 +156,31 @@
})
}
+// TestReloadOnlyOnce checks that changes to the go.mod file do not result in
+// redundant package loads (golang/go#54473).
+//
+// Note that this test may be fragile, as it depends on specific structure to
+// log messages around reinitialization. Nevertheless, it is important for
+// guarding against accidentally duplicate reloading.
+func TestReloadOnlyOnce(t *testing.T) {
+ WithOptions(
+ ProxyFiles(workspaceProxy),
+ WorkspaceFolders("pkg"),
+ ).Run(t, workspaceModule, func(t *testing.T, env *Env) {
+ dir := env.Sandbox.Workdir.URI("goodbye").SpanURI().Filename()
+ goModWithReplace := fmt.Sprintf(`%s
+replace random.org => %s
+`, env.ReadWorkspaceFile("pkg/go.mod"), dir)
+ env.WriteWorkspaceFile("pkg/go.mod", goModWithReplace)
+ env.Await(
+ OnceMet(
+ env.DoneWithChangeWatchedFiles(),
+ LogMatching(protocol.Info, `packages\.Load #\d+\n`, 2, false),
+ ),
+ )
+ })
+}
+
// This test checks that gopls updates the set of files it watches when a
// replace target is added to the go.mod.
func TestWatchReplaceTargets(t *testing.T) {
diff --git a/internal/lsp/cache/load.go b/internal/lsp/cache/load.go
index b15c537..84c2e31 100644
--- a/internal/lsp/cache/load.go
+++ b/internal/lsp/cache/load.go
@@ -128,11 +128,14 @@
if ctx.Err() != nil {
return ctx.Err()
}
+
+ // This log message is sought for by TestReloadOnlyOnce.
if err != nil {
event.Error(ctx, eventName, err, tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs)))
} else {
event.Log(ctx, eventName, tag.Snapshot.Of(s.ID()), tag.Directory.Of(cfg.Dir), tag.Query.Of(query), tag.PackageCount.Of(len(pkgs)))
}
+
if len(pkgs) == 0 {
if err == nil {
err = fmt.Errorf("no packages returned")
@@ -212,6 +215,7 @@
if existing := s.meta.metadata[m.ID]; existing == nil || !existing.Valid {
updates[m.ID] = m
updatedIDs = append(updatedIDs, m.ID)
+ delete(s.shouldLoad, m.ID)
}
}
diff --git a/internal/lsp/regtest/expectation.go b/internal/lsp/regtest/expectation.go
index 7867af9..2649001 100644
--- a/internal/lsp/regtest/expectation.go
+++ b/internal/lsp/regtest/expectation.go
@@ -336,7 +336,11 @@
}
// LogMatching asserts that the client has received a log message
-// of type typ matching the regexp re.
+// of type typ matching the regexp re a certain number of times.
+//
+// The count argument specifies the expected number of matching logs. If
+// atLeast is set, this is a lower bound, otherwise there must be exactly cound
+// matching logs.
func LogMatching(typ protocol.MessageType, re string, count int, atLeast bool) LogExpectation {
rec, err := regexp.Compile(re)
if err != nil {
