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

Jump to bottom

x/tools/gopls: file watchers should be reevaluated after load #60340

Open
findleyr opened this issue May 22, 2023 · 1 comment
Open

x/tools/gopls: file watchers should be reevaluated after load #60340

findleyr opened this issue May 22, 2023 · 1 comment
Labels
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.13.0

Comments

@findleyr
Copy link
Contributor

Normally I would just fix this bug, but given our proximity to the v0.12.0 release I'm filing it for later:

When state changes in gopls, we re-evaluate file watchers. Currently, this is done synchronously to the state change processing, via Server.didModifyFiles->updateWatchedDirectories.

Problems with this:

  • AFAIK there is no need to block change processing on updating watched directories. File watching is always going to be asynchronous, so I don't think we avoid any races by blocking while processing didChange notifications
  • we should generally avoid blocking on server->client calls during notification processing, as it can lead to distributed deadlocks in certain clients
  • we should awaitLoaded before re-evaluating file watches, as go list might surface new directories to watch after e.g. a go.mod change. We can't awaitLoaded during change processing.
@findleyr findleyr added this to the gopls/v0.13.0 milestone May 22, 2023
@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 May 22, 2023
@gopherbot
Copy link

Change https://go.dev/cl/496879 mentions this issue: gopls/internal/lsp/regtest: delete TestWatchReplaceTargets

gopherbot pushed a commit to golang/tools that referenced this issue May 22, 2023
@findleyr
gopls/internal/lsp/regtest: delete TestWatchReplaceTargets
9ca66ba 
We don't actually watch replace targets anymore. The way to specify if a
module is being used is by including it in a go.work file.

Looking back on the flakiness, I am pretty sure it was due simply to
type-checking on slow builders, back when we limited each regtest to
20s. This module imports some standard library packages that used to be
slow to type check. I am pretty sure this test would no longer be flaky,
if we still supported the functionality.

While porting over the assertions from this test to TestUseGoWork, I
discovered golang/go#60340, a bug in the order of our file watcher
evaluation.

Fixes golang/go#50748

Change-Id: I26c10ac659d0f195da18b6181b54d7c373cc984b
Reviewed-on: https://go-review.googlesource.com/c/tools/+/496879
Run-TryBot: Robert Findley <rfindley@google.com>
Reviewed-by: Alan Donovan <adonovan@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
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.13.0
Development

No branches or pull requests
2 participants
@gopherbot @findleyr