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: race conditions result in confusing error message #34410

Closed
stamblerre opened this issue Sep 19, 2019 · 7 comments
Closed

x/tools/gopls: race conditions result in confusing error message #34410

stamblerre opened this issue Sep 19, 2019 · 7 comments
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
Unreleased

Comments

@stamblerre
Copy link
Contributor

Forked from microsoft/vscode-go#2759, as well as several messages on Slack.

This is a fairly unreproducible bug, but it seems that we have a race condition wherein a package may be type-checked with 2 versions of the same dependency. This seems to happen more frequently with packages with test variants. It results in error messages that read something like "type X cannot be used as type X in ...".
@gopherbot gopherbot added this to the Unreleased milestone Sep 19, 2019
@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 Sep 19, 2019
@golang golang deleted a comment from gopherbot Sep 19, 2019
@gopherbot
Copy link

Change https://golang.org/cl/196537 mentions this issue: internal/lsp: add handling to make sure we never re-check an import

gopherbot pushed a commit to golang/tools that referenced this issue Sep 19, 2019
@stamblerre
internal/lsp: make sure that deps are only checked in trimmed mode
db1d4ed 
This change encodes an invariant that, a dependency package will only
ever be parsed with trimmed ASTs.

Updates golang/go#34410

Change-Id: I2ceab3672c0bae0b98cec2a8e60b92a0c01a900f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196537
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@stamblerre
Copy link
Contributor Author

After investigating yesterday, the issue here seems to be that, with CL 186839, the deletion of an item from a map doesn't necessarily cause it to be garbage collected, so we may end up re-using an old cache entry when we wanted to force the type checker to recompute the entry. This will be fixed when we improve our cache invalidation strategy.

@atombender
Copy link

I'm getting something that seems to be a race condition. Could this be the same cause?

While editing code, I'll suddenly get nonsensical errors such as:

switch intent.Status {
  case stripe.PaymentIntentStatusSucceeded:
^ cannot compare stripe.PaymentIntentStatusSucceeded == intent.Status (mismatched types stripe-go.PaymentIntentStatus and stripe-go.PaymentIntentStatus)

...which indicates that it has two sets of type information for the same imported package. The imported package has not been updated, so I don't know why gopls would keep two sets.

The problem manifests as type errors (types not assignable, comparable, ertc.) and import errors.

gopls output here.

I'd be happy to report as separate issue.

Running 5eefd052ad727afc5e2e7b034c752b9d3d902b3b.

clintjedwards pushed a commit to clintjedwards/tools that referenced this issue Sep 25, 2019
@stamblerre @clintjedwards
internal/lsp: make sure that deps are only checked in trimmed mode
9219836 
This change encodes an invariant that, a dependency package will only
ever be parsed with trimmed ASTs.

Updates golang/go#34410

Change-Id: I2ceab3672c0bae0b98cec2a8e60b92a0c01a900f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/196537
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@sebsebmc sebsebmc mentioned this issue Sep 30, 2019
Closed
@myitcv
Copy link
Member

myitcv commented Oct 1, 2019 •
edited

Also seeing this within govim using gopls and x/tools a8d5d34

@gopherbot
Copy link

Change https://golang.org/cl/198317 mentions this issue: internal/lsp: use dependencies in cache keys

gopherbot pushed a commit to golang/tools that referenced this issue Oct 3, 2019
@stamblerre
internal/lsp: use dependencies in cache keys
c56b4b1 
This change includes dependencies in the cache keys for
CheckPackageHandles. This should fix the issue with propagating results
to reverse dependencies.

Updates golang/go#34410

Change-Id: I025b7f0d6b0dcaa89c3461ebd94eadd35de4955f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/198317
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@stamblerre
Copy link
Contributor Author

This error should be resolved as of the above CL.

@zikaeroh
Copy link
Contributor

zikaeroh commented Oct 3, 2019

I'm assuming #34584 should also be closed, since it looks to be the same issue?

gopherbot pushed a commit to golang/tools that referenced this issue Oct 4, 2019
@stamblerre @bradfitz
internal/lsp: use dependencies in cache keys
9901302 
This change includes dependencies in the cache keys for
CheckPackageHandles. This should fix the issue with propagating results
to reverse dependencies.

Updates golang/go#34410

Change-Id: I025b7f0d6b0dcaa89c3461ebd94eadd35de4955f
Reviewed-on: https://go-review.googlesource.com/c/tools/+/198317
Run-TryBot: Rebecca Stambler <rstambler@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Ian Cottrell <iancottrell@google.com>
@myitcv myitcv mentioned this issue Oct 9, 2019
Closed
@ramya-rao-a ramya-rao-a mentioned this issue Nov 10, 2019
Closed
@golang golang locked and limited conversation to collaborators Oct 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
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
Unreleased
Development

No branches or pull requests
5 participants
@atombender @myitcv @stamblerre @gopherbot @zikaeroh