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

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
Milestone

Comments

@stamblerre
Copy link
Contributor

@stamblerre stamblerre commented Sep 19, 2019

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
@golang golang deleted a comment from gopherbot Sep 19, 2019
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Sep 19, 2019

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
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

This comment has been minimized.

Copy link
Contributor Author

@stamblerre stamblerre commented Sep 20, 2019

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

This comment has been minimized.

Copy link

@atombender atombender commented Sep 23, 2019

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 added a commit to clintjedwards/tools that referenced this issue Sep 25, 2019
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>
@myitcv

This comment has been minimized.

Copy link
Member

@myitcv myitcv commented Oct 1, 2019

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

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Oct 1, 2019

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
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

This comment has been minimized.

Copy link
Contributor Author

@stamblerre stamblerre commented Oct 3, 2019

This error should be resolved as of the above CL.

@stamblerre stamblerre closed this Oct 3, 2019
@zikaeroh

This comment has been minimized.

Copy link

@zikaeroh 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
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.