Temporarily serialize requests in the LSP, and fix the bugs that doing so revealed #3370
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf CI / buf (pull_request).
|
private/buf/bufctl/controller.go
Outdated
| @@ -129,6 +130,7 @@ type Controller interface { | |||
| defaultMessageEncoding buffetch.MessageEncoding, | |||
| options ...FunctionOption, | |||
| ) error | |||
| GetWKTBucket(ctx context.Context) (storage.ReadBucket, error) | |||
There was a problem hiding this comment.
This shouldn't be part of the Controller really - instead, expose bufcli.newWKTStore as public bufcli.NewWKTStore (ie just s/n/N/) and call this function in buflsp to make a bufwktstore.Store.
private/buf/buflsp/file.go
Outdated
| // If this is an external file, it will be in the cache and therefore | ||
| // finding imports via lsp.findImportable() will not be possible. |
There was a problem hiding this comment.
I am not entirely sure I follow this logic -- so a file we may be checking may be a dependency file, and therefore the URI would be to a file in the cache. But I'm not sure why that would then mean we would not be able to call controller.GetImportableFileInfos? It appears to me that we are basically replicating that logic in findImportable (resolving the workspace using the controller, walking the files, and adding WKTs)...
There was a problem hiding this comment.
GetImportableFileInfos and findImportable will both produce "truncated" paths when looking up the workspace of a cached file, presumably because it does not have a buf.yaml. I think trying to fix that is beyond the scope of this PR, so I'm working around it. A comment will appear soon explaining it.
There was a problem hiding this comment.
Ah, I think I might know what's going on. In the case of the cache, we do not have a way to show the root of the workspace (e.g. there are no configuration files), so when resolving the workspace for the given file path, it is truncating it to that directory. There may be a way to fix this by special-casing paths within the cache, since those are known modules.
|
What's the status on this one? |
|
I'm currently stuck debugging the issue Doria pointed out (insofar that I think there's a way I'm using workspaces wrong but docs are a bit paltry), but unfortunately the logging migration appears to have broken debugging for the LSP (and broken some other things along with it? still triaging). Not sure I can give an ETA on that. |
|
I'm not sure how the logging would have impacted it, but if you can point out specifics happy to help. The changes were largely superficial, however. |
|
The biggest issue was that the function https://github.com/bufbuild/buf/pull/3370/files#diff-9fde706d21f072dfdf6f3fc98fc5dd6035d8aca46905dc8d6e819cf3cdde4f5bR335 was deleted with no replacement. slog does not natively know how to log arbitrary types, and passing such values into Also there were several unfortunate merge conflicts because the logging upgrade touched every log statement in the LSP, so merging main introduced additional bugs I had to track down while fixing logging. |
Fixes #3362, #3359, #3361.
Previously, the LSP contained a somewhat fussy concurrency model, where files had to be carefully locked and unlocked (with runtime enforcement) to avoid deadlocks. This is necessary so that parsing and resolution does not hang the LSP for particularly long: you want to avoid a situation where a user opens a file that takes several seconds to load, which is a realistic possibility.
This does not hang the editor itself, but prevents go-to-definition from working on already-loaded files while the new file is loading. Already with only needing to go three or four levels deep, as in the protos in
buf-registry, causes a noticeable pause. I have not profiled it, but I suspect it's because the CLI is not optimized for memoization, so the memoization that I'm doing probably duplicates work other parts of the CLI are doing.However, the current solution was understood to be temporary, since Josh and I are designing a new query system for incremental compilation, which would replace the current system. Also, the current system is quite painful to debug, despite the many steps I have taken to enforce locking invariants.
Therefore, given that:
it makes sense to switch to a global lock to serialize all requests until (1). Removing (2) will make it easier to fix "low hanging" bugs in the LSP, instead of blocking on (1) happening. If we start pushing the LSP to users before (1), we should beware of (3).
As a result of switching to serialized requests, #3362 disappears in a puff of "this problem no longer exists". It also revealed that the way dependency paths were produced by
GetImportableImageFileInfoswas subtly incorrect if the input file path was inside of the cache. Fixing this resolved #3359. Because we now we have crisp information about where imports come from, it is trivial to resolve #3361.