internal/lsp: improve error handling while parsing #110
Conversation
muirdm
force-pushed
the
lsp-handle-cancel-during-parse
branch
from
839fe65
to
5cb21f3
Compare
|
This PR (HEAD: 5cb21f3) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/tools/+/181317 to see it. Tip: You can toggle comments from me using the |
|
Message from Muir Manders: Patch Set 1: I think this might fix most of the "no package for file" issues. Note that the package test variant bug can still cause test files to be broken. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Rebecca Stambler: Patch Set 1: (1 comment) Thanks for catching these! Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
muirdm
force-pushed
the
lsp-handle-cancel-during-parse
branch
from
5cb21f3
to
382d6bc
Compare
|
This PR (HEAD: 382d6bc) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/tools/+/181317 to see it. Tip: You can toggle comments from me using the |
muirdm
force-pushed
the
lsp-handle-cancel-during-parse
branch
from
382d6bc
to
5b76a5a
Compare
|
This PR (HEAD: 5b76a5a) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/tools/+/181317 to see it. Tip: You can toggle comments from me using the |
|
Message from Muir Manders: Patch Set 3: (1 comment)
My repro apparently wasn't 100% because I've now realized getPkg was still leaving broken entries in the view.pcache by caching the error. No files actually map to the error'd pcache entry, so the view.invalidateContent logic will never invalidate that cache entry. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Rebecca Stambler: Patch Set 4: (6 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
muirdm
force-pushed
the
lsp-handle-cancel-during-parse
branch
from
5b76a5a
to
e72c574
Compare
|
This PR (HEAD: e72c574) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/tools/+/181317 to see it. Tip: You can toggle comments from me using the |
|
Message from Muir Manders: Patch Set 7: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Muir Manders: Patch Set 7: (3 comments) Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Muir Manders: Patch Set 9: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Rebecca Stambler: Patch Set 10: Run-TryBot+1 Code-Review+2 Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Gobot Gobot: Patch Set 10: TryBots beginning. Status page: https://farmer.golang.org/try?commit=b7c4541e Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Gobot Gobot: Patch Set 10: Build is still in progress... Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
muirdm
force-pushed
the
lsp-handle-cancel-during-parse
branch
from
e72c574
to
ad0d595
Compare
|
This PR (HEAD: ad0d595) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/tools/+/181317 to see it. Tip: You can toggle comments from me using the |
|
Message from Muir Manders: Patch Set 12: Commit message was updated. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Gobot Gobot: Patch Set 10: Build is still in progress... Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Rebecca Stambler: Patch Set 13: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Gobot Gobot: Patch Set 13: TryBots beginning. Status page: https://farmer.golang.org/try?commit=c3aec4d2 Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Gobot Gobot: Patch Set 13: Build is still in progress... Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Rebecca Stambler: Patch Set 13: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Gobot Gobot: Patch Set 13: Build is still in progress... Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Gobot Gobot: Patch Set 13: Build is still in progress... Other builds still in progress; subsequent failure notices suppressed until final report. Consult https://build.golang.org/ to see whether they are new failures. Keep in mind that TryBots currently test exactly your git commit, without rebasing. If your commit's git parent is old, the failure might've already been fixed. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
If the context is canceled (or times out) during parsing, we were previously caching the package with no *ast.Files. Any further LSP queries against that package would fail because the package is already loaded, but none of the files are mapped to the package. Fix by propagating non-parse errors as "fatal" errors in parseFiles. typeCheck will propagate these errors and not cache the package. I also fixed the package cache to not cache errors loading packages. If you get an error like "context canceled" then none of the package's files are mapped to the package. This prevents the package from ever getting unmapped when its files are updated. I also added a retry mechanism where if the current request is not canceled but the package failed to load due to a previous request being canceled, this request can try loading the package again.
muirdm
force-pushed
the
lsp-handle-cancel-during-parse
branch
from
ad0d595
to
5f1e7ef
Compare
|
This PR (HEAD: 5f1e7ef) has been imported to Gerrit for code review. Please visit https://go-review.googlesource.com/c/tools/+/181317 to see it. Tip: You can toggle comments from me using the |
|
Message from Rebecca Stambler: Patch Set 15: Run-TryBot+1 Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Gobot Gobot: Patch Set 15: TryBots beginning. Status page: https://farmer.golang.org/try?commit=6b1241ec Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
|
Message from Gobot Gobot: Patch Set 15: TryBot-Result+1 TryBots are happy. Please don’t reply on this GitHub thread. Visit golang.org/cl/181317. |
If the context is canceled (or times out) during parsing, we were previously caching the package with no *ast.Files. Any further LSP queries against that package would fail because the package is already loaded, but none of the files are mapped to the package. Fix by propagating non-parse errors as "fatal" errors in parseFiles. typeCheck will propagate these errors and not cache the package. I also fixed the package cache to not cache errors loading packages. If you get an error like "context canceled" then none of the package's files are mapped to the package. This prevents the package from ever getting unmapped when its files are updated. I also added a retry mechanism where if the current request is not canceled but the package failed to load due to a previous request being canceled, this request can try loading the package again. Updates golang/go#32354, golang/go#32360 Change-Id: I466ddb8d336aeecf6e50f9f6d040787a86a60ca0 GitHub-Last-Rev: 5f1e7ef GitHub-Pull-Request: #110 Reviewed-on: https://go-review.googlesource.com/c/tools/+/181317 Run-TryBot: Rebecca Stambler <rstambler@golang.org> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Rebecca Stambler <rstambler@golang.org>
|
This PR is being closed because golang.org/cl/181317 has been merged. |
If the context is canceled (or times out) during parsing, we were
previously caching the package with no *ast.Files. Any further LSP
queries against that package would fail because the package is already
loaded, but none of the files are mapped to the package. Fix by
propagating non-parse errors as "fatal" errors in
parseFiles. typeCheck will propagate these errors and not cache the
package.
I also fixed the package cache to not cache errors loading
packages. If you get an error like "context canceled" then none of the
package's files are mapped to the package. This prevents the package
from ever getting unmapped when its files are updated. I also added a
retry mechanism where if the current request is not canceled but the
package failed to load due to a previous request being canceled, this
request can try loading the package again.
Updates golang/go#32354, golang/go#32360