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: TestUseGoWork/experimental flaking with "no package for given file position" #53733

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

x/tools/gopls: TestUseGoWork/experimental flaking with "no package for given file position" #53733

findleyr opened this issue Jul 7, 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
Member

findleyr commented Jul 7, 2022

TestUseGoWork is flaking at master with "no package for given file position"

greplogs --dashboard -md -l -e 'FAIL: TestUseGoWork' --since=2022-07-01

2022-07-06T19:31:10-d69bac6-3da88c0/linux-amd64-buster
2022-07-06T00:02:11-afa4a95-2007599/linux-amd64-unified
2022-07-04T04:09:44-698251a-3cf79d9/linux-amd64
2022-07-04T04:09:44-698251a-3cf79d9/linux-amd64-sid
2022-07-01T17:30:37-698251a-e822b1e/linux-amd64-longtest

This is definitely a gopls bug / race condition, albeit one that probably only manifests when experimentalUseInvalidMetadata is set. It is likely to have been a long-standing race, that was only exposed by CL 411913 (as explained in https://go-review.googlesource.com/c/tools/+/411913/comments/d750a369_d1c3110c).

Somehow, due to racing loads, we're left in a state where package a depends on a package b that no longer exists.
@findleyr findleyr added this to the gopls/v0.9.1 milestone Jul 7, 2022
@findleyr findleyr self-assigned this Jul 7, 2022
@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 7, 2022
@bcmills bcmills mentioned this issue Jul 7, 2022
Closed
@findleyr
Copy link
Member Author

findleyr commented Jul 7, 2022

After much head scratching, I understand this flake now. There are many races when we allow building packages with invalid metadata, but I will summarize as follows:

  • For a given snapshot, we only set new metadata if the old metadata was invalid. Generally, this preserves an invariant that if we build a package in a snapshot, it will match the metadata we have for that package.
  • ...but if experimentalUseInvalidMetadata is set, this invariant is broken. In particular, due to our fine grained locking we can observe the following sequence of events:
    • Operation A tries to build a package handle with invalid metadata.
    • A load sets valid metadata
    • Operation A saves the package handle even though its metadata does not match the loaded metadata.

I am skeptical of whether it is worth supporting this mode of using invalid metadata. go.dev/issues/42266 does not have too many details regarding what problem this solves.

...but in the meantime I will try simply making it impossible to add a package that does not match our current metadata for its package ID. I suspect that will make the flakes go away (and makes things more correct, after all).

@findleyr
Copy link
Member Author

findleyr commented Jul 7, 2022

CC @adonovan and @heschi who may have opinions on what is correct here.

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/416224 mentions this issue: internal/lsp/cache: fail addPackageHandle if metadata is stale

gopherbot pushed a commit to golang/tools that referenced this issue Jul 8, 2022
@findleyr
internal/lsp/cache: fail addPackageHandle if metadata is stale
9c2a556 
If metadata is refreshed during the execution of buildPackageHandle, we
should not store the resulting package handle in the snapshot, as it
breaks the invariant that computed packages match the currently loaded
metadata.

This strictness revealed another bug: because of our fine-grained
locking in snapshot.load, it is possible that we set valid metadata
multiple times, leading to unnecessary invalidation and potential
further races to package handles. Fix this by building all metadata when
processing the go/packages result, and only filtering updates after
grabbing the lock.

For golang/go#53733

Change-Id: Ib63ae4fbd97d0d25d45fe04f9bcd835996db41da
Reviewed-on: https://go-review.googlesource.com/c/tools/+/416224
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>
@findleyr
Copy link
Member Author

findleyr commented Jul 8, 2022

I think https://go.dev/cl/416224 will fix this bug, but since it introduces more assertions I'd like to wait and see that flakes are fully eliminated.

@bcmills
Copy link
Contributor

bcmills commented Jul 11, 2022

So far so good: none since July 7.

greplogs --dashboard -md -l -e 'FAIL: TestUseGoWork' --since=2022-07-07
2022-07-07T16:57:02-1dfab61-3a7cec2/linux-amd64-goamd64v3
2022-07-07T14:56:05-2aef121-3a7cec2/linux-amd64-wsl

@findleyr
Copy link
Member Author

Closing as I am fairly certain this is fixed.

@findleyr findleyr modified the milestones: gopls/v0.9.1, gopls/v0.9.2 Jul 13, 2022
@bcmills bcmills mentioned this issue Jul 18, 2022
Closed
@golang golang locked and limited conversation to collaborators Jul 13, 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
@bcmills @gopherbot @findleyr