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.

Sign up for GitHub

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

Jump to bottom

x/tools/gopls: TestCommandLine failing frequently with duplicate references #53796

Closed
findleyr opened this issue Jul 11, 2022 · 6 comments
Closed

x/tools/gopls: TestCommandLine failing frequently with duplicate references #53796

findleyr opened this issue Jul 11, 2022 · 6 comments
Assignees
@findleyr
Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone
gopls/v0.9.2

Comments

@findleyr
Copy link
Contributor

Many test flakes since https://go.dev/cl/416874. Investigating briefly to see if rolling back can be avoided.
@gopherbot gopherbot added Tools This label describes issues relating to any tools in the x/tools repository. gopls Issues related to the Go language server, gopls. labels Jul 11, 2022
@gopherbot gopherbot added this to the Unreleased milestone Jul 11, 2022
@findleyr findleyr changed the title x/tools/gopls: TestCommandLine/Modules failing frequently with duplicate references x/tools/gopls: TestCommandLine failing frequently with duplicate references Jul 11, 2022
@gopherbot
Copy link

Change https://go.dev/cl/416877 mentions this issue: internal/lsp/cache: use mod=readonly for

@gopherbot
Copy link

Change https://go.dev/cl/416879 mentions this issue: Revert "internal/lsp/cache: don't pin a snapshot to view.importsState"

gopherbot pushed a commit to golang/tools that referenced this issue Jul 11, 2022
@findleyr @gopherbot
Revert "internal/lsp/cache: don't pin a snapshot to view.importsState"
b6e4951 
This reverts commit 42457a5.

Reason for revert: test flakes (golang/go#53796)

Change-Id: I9d7061220b43f9de88060a0bba5c5635d92fe3d9
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416879
Reviewed-by: Alan Donovan <adonovan@google.com>
Auto-Submit: Robert Findley <rfindley@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
gopls-CI: kokoro <noreply+kokoro@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@findleyr
Copy link
Contributor Author

CC @adonovan

Decided to revert CL 416874 as the root cause was initially opaque: how could a change relating to imports ProcessEnv cause failures in GOPATH command-line tests?!

But thinking about this a bit more, I believe I have a likely hypothesis:

  • refs.go is part of a test variant, and yet the reference de-duplication uses token.Pos
  • Now that we no longer leak the snapshot, it seems possible that shutdown races cause a parsedGoFile to be evicted from the cache while a package handle remains.
  • If the next test gets a cache hit for the packageHandle, we can end up with two packages whose parsed go files do not match.

So the test failure can be the result of snapshot shutdown actually working properly... note that while the test runner delays shutdown until the end, unfortunately the lsprpc package issues a shutdown itself.

@findleyr findleyr modified the milestones: Unreleased, gopls/v0.9.1 Jul 12, 2022
@gopherbot
Copy link

Change https://go.dev/cl/416881 mentions this issue: internal/lsp/source: use token.File-agnostic positions to dedupe refs

@adonovan
Copy link
Member

Does the second test happen after the first one completes shutdown? Or is the point that shutdown is asynchronous and therefore the second test races with cache eviction?

There is something to be said for relying more heavily on integration testing of the executable through its user interface (i.e. the LSP service). They may be slower to start up but such tests tend to exhibit fewer artifacts of the testing framework. And slow startup can usually be fixed.

gopherbot pushed a commit to golang/tools that referenced this issue Jul 12, 2022
@findleyr
internal/lsp/source: use token.File-agnostic positions to dedupe refs
bc957ec 
We were already using a token.File-agnostic position for object
positions in our references algorithm, but still relied on token.Pos for
identifying duplicate references. This breaks down when a file may have
multiple parsed representations in different packages.

While we should endeavor to eliminate duplicate parsing, our algorithms
should not rely on this for correctness. Update the reference
de-duplication to use the same position key as object search.

For golang/go#53796

Change-Id: Ic2e6c23380ea4e6b2747e4e5b45d7bfa6e656f0f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416881
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Alan Donovan <adonovan@google.com>
gopherbot pushed a commit to golang/tools that referenced this issue Jul 12, 2022
@findleyr
Revert "Revert "internal/lsp/cache: don't pin a snapshot to view.impo…
a79ee0f 
…rtsState"

This reverts CL 416879, which itself was a revert of CL 416874.

Reason for revert: failure is understood now, as described in 
golang/go#53796 (comment) and fixed in CL 416881.

Change-Id: I1d6a4ae46fbb1bf78e2f23656de7885b439f43fb
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416882
Reviewed-by: Alan Donovan <adonovan@google.com>
Run-TryBot: Robert Findley <rfindley@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@findleyr
Copy link
Contributor Author

Does the second test happen after the first one completes shutdown? Or is the point that shutdown is asynchronous and therefore the second test races with cache eviction?

Shutdown is asynchronous: the test closes the connection and the server is configured to asynchronously clean up resources, even if the test runner does not issue the 'shutdown' RPC. So the cache eviction races with the next test.

We could improve this, but fundamentally the cache should only be an optimization, and should not affect correctness. https://go.dev/cl/416881 fixes the correctness bug. Closing this issue as the fundamental problem is fixed.

@findleyr findleyr modified the milestones: gopls/v0.9.1, gopls/v0.9.2 Jul 13, 2022
@findleyr findleyr self-assigned this Aug 8, 2022
@golang golang locked and limited conversation to collaborators Aug 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@findleyr findleyr

Labels
FrozenDueToAge gopls Issues related to the Go language server, gopls. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Milestone
gopls/v0.9.2
Development

No branches or pull requests
3 participants
@adonovan @gopherbot @findleyr