Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x/tools/gopls: reload metadata on the most durable awaiting context #43652

Open
findleyr opened this issue Jan 12, 2021 · 1 comment
Open

x/tools/gopls: reload metadata on the most durable awaiting context #43652

findleyr opened this issue Jan 12, 2021 · 1 comment

Comments

@findleyr
Copy link
Contributor

@findleyr findleyr commented Jan 12, 2021

We try to avoid cloning a snapshot when the previous snapshot has not initialized, by awaiting on a detached context:
https://cs.opensource.google/go/x/tools/+/master:internal/lsp/cache/view.go;l=602;drc=929a8494cf60267d89035bc211b797a67b65a6b9

However, we also guard initialization with a sync.Once. As has been observed in #43554, it's possible to lose this race to awaitInitialized, such that we run initialization on the snapshot background context, which can be canceled.

As a result, we can clone a snapshot with incomplete metadata. This causes problems for any invalidation logic trying to preserve metadata, such as only invalidating metadata on go.mod saves. Additionally, this was difficult to debug: it would be nice to have a guarantee that we have a linear history of metadata in our snapshots.

After discussing, we think this is a relatively low priority since the current logic should only result in rare and transient breakages.

CC @stamblerre @heschik

@gopherbot gopherbot added this to the Unreleased milestone Jan 12, 2021
@findleyr findleyr removed this from the Unreleased milestone Jan 12, 2021
@findleyr findleyr added this to the gopls/v1.0.0 milestone Jan 12, 2021
@gopherbot
Copy link

@gopherbot gopherbot commented Jan 13, 2021

Change https://golang.org/cl/283512 mentions this issue: gopls/internal/regtest: fix synchronization for TestUseGoplsMod

gopherbot pushed a commit to golang/tools that referenced this issue Jan 13, 2021
Jumping to definition in a regtest can indirectly lead to a didOpen
call, so the awaits added to TestUseGoplsMod to synchronize metadata
were ineffectual. On Android, this can lead to the race described in
golang/go#43652.

But in any case, all this bookkeeping of notifications is fragile. Avoid
it entirely by having the fake editor keep track of notification
statistics. In the future, we should use this to clean up many existing
regtests.

For golang/go#43554
For golang/go#39384

Change-Id: Icd1619bd5cdd2f646d1a0050f5beaf2ab1c27f37
Reviewed-on: https://go-review.googlesource.com/c/tools/+/283512
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
Trust: Robert Findley <rfindley@google.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Rebecca Stambler <rstambler@golang.org>
@stamblerre stamblerre added this to To Do in gopls on-deck Feb 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants