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

[findleyr]

Many test flakes since https://go.dev/cl/416874. Investigating briefly to see if rolling back can be avoided.

[gopherbot]

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

[gopherbot]

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

[findleyr]

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.

[gopherbot]

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

[adonovan]

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.

[findleyr]

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.