x/tools/gopls: diagnostics not sent for new buffer #35638
Labels
FrozenDueToAge
gopls
Issues related to the Go language server, gopls.
NeedsInvestigation
Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Tools
This label describes issues relating to any tools in the x/tools repository.
Milestone
Comments
myitcv
added
NeedsInvestigation
gopherbot
added
the
Tools
|
This appears to have started as a result of https://go-review.googlesource.com/c/tools/+/206888 |
|
I'm done for the day, but this looks a lot like #35949. |
|
We're still seeing this as of 259af5f. |
|
@myitcv: which govim test is it? format_on_save_new_file_existing_package doesn't seem to exist any more. |
|
@heschik: it's exactly the one you mention, but you need to be looking on this branch: govim/govim#584 Output |
|
Thanks, repros nicely. I think this is metadata corruption: We try to generate diagnostics for |
|
Change https://golang.org/cl/210559 mentions this issue: |
|
In a similar vein, the following with what appears to be the root cause of: |
|
When I run that test I get the following output: I don't see any reason errors from |
|
Per my Slack message, I'm still seeing the |
That's just the problem; they're not in the errors list, and that's why the test is failing. Hence why I think it's related to/the same as this issue.
I'll wait until we have a resolution on this issue first. |
|
Ah, got it. Maybe, I'll take a look tomorrow. |
|
I think that On my workstation |
Ah, good spot, I see what's happened here. We refactored diagnostic handling recently and the |
|
Fun. We load, discover const.go in mod.com, and then...immediately forget? This is going to take more iteration. I'm still kind of a GitHub noob. Is triggering a Travis build as simple as sending a PR, or do I need some permissions? |
Yes. So fork |
|
Thanks for all the testing. I'm not sure we know the precise problem, but we have a pretty good theory. When you open a new file, we need to reload the packages that it belongs to. To do that, we look at all the packages in the workspace, and heuristically invalidate any that have files in the same directory as the new file. That requires looking through the existing metadata. That happens in the However, the reads for the invalidation happen in a different critical section than I thought that the trigger in this test was the initial workspace load, which runs diagnostics concurrently with other operations, so I tried to make that synchronous in the latest patchsets. But either I didn't succeed, I was wrong about the trigger, or the theory is bad. |
|
Thanks for the update, @heschik. Also significant to my mind is that if you look at a failing log of |
|
Sorry, uploaded wrong log files in previous comment for anyone following along (closely). Re-uploaded and updated previous comment. |
|
One critical-looking issue appears to be this line: Note the logs says it's publishing diagnostics for |
|
Not to steal @heschik's thunder, but we've made a bit of progress. I've just pushed up to govim/govim#584 (our PR for upgrading I've also fixed up other tests that were incorrectly, following CL 208261, waiting on diagnostics to be sent when they haven't changed. Locally I haven't been able to reproduce a breaking https://travis-ci.org/govim/govim/builds/624989118 The one test that is now failing relatively regularly is |
This was referenced Dec 14, 2019
|
Further status update:
However at this stage, with a fix for the first point applied, and given the rarity of the second point, we currently have a green |
|
Change https://golang.org/cl/211537 mentions this issue: |
|
Change https://golang.org/cl/211578 mentions this issue: |
gopherbot
pushed a commit
to golang/tools
that referenced
this issue
Dec 16, 2019
Cloning is complicated enough without worrying about concurrency, so hold the snapshot's lock during the entire process. Consolidate everything into one function. I don't think that the split was making it easier to understand, and I was able to see and clean up some extra complexity once it was all in one place. Let's discuss options if you think the result is too long. I don't intend any semantic changes in this CL. Updates golang/go#35638. Change-Id: I05c4b28875976293f5fcd56248d9c9e468f85cc6 Reviewed-on: https://go-review.googlesource.com/c/tools/+/211537 Run-TryBot: Heschi Kreinick <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Rebecca Stambler <[email protected]>
|
@myitcv, can you confirm that https://golang.org/cl/211578 works? |
myitcv
pushed a commit
to myitcvforks/tools
that referenced
this issue
Dec 16, 2019
Cloning is complicated enough without worrying about concurrency, so hold the snapshot's lock during the entire process. Consolidate everything into one function. I don't think that the split was making it easier to understand, and I was able to see and clean up some extra complexity once it was all in one place. Let's discuss options if you think the result is too long. I don't intend any semantic changes in this CL. Updates golang/go#35638. Change-Id: I05c4b28875976293f5fcd56248d9c9e468f85cc6
myitcv
pushed a commit
to myitcvforks/tools
that referenced
this issue
Dec 16, 2019
The situation in golang/go#35638 was as follows: didOpen main.go creates a snapshot that knows main.go is in package "mod.com". didChange main.go creates a snapshot. When a file changes, we discard its contents by leaving the file handle out of the "files" map. didOpen const.go creates a snapshot, and attempts to invalidate the metadata for packages in the same directory. The way we detect packages in the same directory is by iterating through the files in the snapshot. But we threw away the only file in "mod.com" in step 2 when its contents changed. If a diagnostics run happened to get in between the two steps, it would re-load main.go and the bug would go away. If not, step 3 would find no files and fail to invalidate "mod.com". The best way to fix this is to insert the new file handle eagerly during cloning. That way there's no confusion. Fixes golang/go#35638. Change-Id: I340bd28a96ad7b4cc912032065f3c2732c380bb2
|
Looks good so far: https://travis-ci.org/govim/govim/builds/625918838?utm_medium=notification&utm_source=github_status. Note the build would almost certainly have failed pre the race tests if the fix was not good. Just need a fix for #36142 now. @stamblerre - does adding a configuration option along the lines discussed in #36142 (comment) make sense to you too? |
myitcv
pushed a commit
to myitcvforks/tools
that referenced
this issue
Dec 17, 2019
The situation in golang/go#35638 was as follows: didOpen main.go creates a snapshot that knows main.go is in package "mod.com". didChange main.go creates a snapshot. When a file changes, we discard its contents by leaving the file handle out of the "files" map. didOpen const.go creates a snapshot, and attempts to invalidate the metadata for packages in the same directory. The way we detect packages in the same directory is by iterating through the files in the snapshot. But we threw away the only file in "mod.com" in step 2 when its contents changed. If a diagnostics run happened to get in between the two steps, it would re-load main.go and the bug would go away. If not, step 3 would find no files and fail to invalidate "mod.com". The best way to fix this is to insert the new file handle eagerly during cloning. That way there's no confusion. Fixes golang/go#35638. Change-Id: I340bd28a96ad7b4cc912032065f3c2732c380bb2
myitcv
pushed a commit
to myitcvforks/tools
that referenced
this issue
Dec 17, 2019
The situation in golang/go#35638 was as follows: didOpen main.go creates a snapshot that knows main.go is in package "mod.com". didChange main.go creates a snapshot. When a file changes, we discard its contents by leaving the file handle out of the "files" map. didOpen const.go creates a snapshot, and attempts to invalidate the metadata for packages in the same directory. The way we detect packages in the same directory is by iterating through the files in the snapshot. But we threw away the only file in "mod.com" in step 2 when its contents changed. If a diagnostics run happened to get in between the two steps, it would re-load main.go and the bug would go away. If not, step 3 would find no files and fail to invalidate "mod.com". The best way to fix this is to insert the new file handle eagerly during cloning. That way there's no confusion. Fixes golang/go#35638. Change-Id: I340bd28a96ad7b4cc912032065f3c2732c380bb2
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
We have a
govimtest that starts with a module containing the following file (on disk):Then it creates a new buffer for
const.go(i.e. does not exist on disk initially) and populates it with the following content:Then it saves
const.go, triggeringcodeAction(forgoimports-like fixing) followed byFormatting.What did you expect to see?
const.goCodeActioncall to not return anilerror and no editsFormattingcall to return anilerror an edits to remove the first blank lineWhat did you see instead?
const.goCodeActioncall returns an error:goplsis also logging to stderr:Also of note is that the initial
Configurationcall to the client (govim) happens aftergovimhas made a number of calls togopls. This has certainly changed from before where config was always called first, with effectively any other activity being blocked on it returning (or at least that's how it appeared to work).See
goplslog: fail.logSeparately, this issue appears to be related to #35114 because sometimes we see the following on
stderr:cc @stamblerre @matloob
The text was updated successfully, but these errors were encountered: