`Commands.state` can contain invalid data
In the "Auto-Open from completion" tests (for #788) I forgot to Cache the created LSP Server. But after adding Async.Cache some tests suddenly failed. Even worse: In different test runs different tests failed.... Test failure was: Returned open position was different from expected position.
So I investigated:
-
Commands.Completion(textDocument/completionrequest) returns immediately all found Completion items, and triggers a background collection of details (like Namespace insert location) for the completions. -
Commands.HelpText(completionItem/resolverequest) then returns the actual details for a specific Item (Regexin tests). Depending on the progress of the background collection (triggered inCommands.Completion) the details might come from cache (state.HelpText) or are generated on the fly.
BUT: Because the cache is filled in the background, a new incoming Completion request might invalidate the current cache. But the Background Job still fills the cache -- now with outdated data.
To test:
- Get code (https://github.com/Booksbaum/FsAutoComplete/commit/a0993023a25e189f6b384ad4deaefb4a31e4474b):
git clone https://github.com/Booksbaum/FsAutoComplete.git FsAutoCompleteState cd FsAutoCompleteState git checkout a0993023a25e189f6b384ad4deaefb4a31e4474b - Move into dir with test project:
cd test\FsAutoComplete.Tests.Lsp - Run tests:
dotnet run(It runs only a couple of focused tests) - Most likely at least one test will fail (if not: run again)
- I introduced a
versionto track the cache -> look for messages like[13:12:40 ERR] [Test] versions don't match: current v11 <> v10 cached [13:12:40 ERR] [Test] different NsInsert: expected={"Column": 2, "Line": 40, "$type": "<>f__AnonymousType685321008`2"}; cached={"Column": 4, "Line": 34, "$type": "<>f__AnonymousType685321008`2"}
- I introduced a
In sequence diagram:
In Code:
-
Commands.Completiongets called when LSP Server getstextDocument/completion- It clears some cached things in the current state: https://github.com/fsharp/FsAutoComplete/blob/master/src/FsAutoComplete.Core/Commands.fs#L734-L737
- It starts a background job to fill the just cleared data with new data https://github.com/fsharp/FsAutoComplete/blob/master/src/FsAutoComplete.Core/Commands.fs#L360-L374
- -> Background job might still run when
Commands.Completionis called again with a new position
-> caches in state get cleared, but old background job still fills cache -- now with outdated data -
Commands.HelpTexttries to read from cache https://github.com/fsharp/FsAutoComplete/blob/master/src/FsAutoComplete.Core/Commands.fs#L714-L716 (in fact: to get to this code snippet, there are a couple of other cache accesses -- but I focused on the NamespaceInsert part)
state.{Declarations; HelpText CompletionNamespaceInsert; CurrentAst} are only used in Commands.Completion (and its background job) and Commands.HelpText.
But the same issue probably occurs for the other data in state.
It's probably not an issue that occurs often when using the LSP Server in an editor -- after all actions here are mostly human driven ... and as such quite slow. (Even the CI build seems to be 'too slow' and the issue doesn't occur -- only in local test runs (with a decently speedy PC))
For testing I made a simple "fix": Instead of just the namespace as dictionary key, I additional add a id (or version) unique for each Commands.Completion call -> background job still writes old data, but that gets ignored because it has an old id.
That allows the tests to run with a cached server.
See commit https://github.com/Booksbaum/FsAutoComplete/commit/93bc02ecd72869e08337cccbd13584c6777190bc
But it's unfortunately not a real fix: All things are still async and might be out of order. With that id it's just way less likely for the issue to emerge...
(Additional: Currently only some caches in State are gated)