Commands.state can contain invalid data

[Booksbaum]

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/completion request) returns immediately all found Completion items, and triggers a background collection of details (like Namespace insert location) for the completions.
• Commands.HelpText (completionItem/resolve request) then returns the actual details for a specific Item (Regex in tests). Depending on the progress of the background collection (triggered in Commands.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 (Booksbaum@a099302):

  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 version to 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"}
    

In sequence diagram:

• correctly retrieved location from cache und uncached
• incorrect location from cache

In Code:

• Commands.Completion gets called when LSP Server gets textDocument/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.Completion is called again with a new position
  -> caches in state get cleared, but old background job still fills cache -- now with outdated data
• Commands.HelpText tries 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 Booksbaum@93bc02e

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)

[TheAngryByrd]

Since we've moved to AdaptiveLSPServer as default, do you think it's worth keeping this open?

[Booksbaum]

Yes, I think we can consider this here outdated. AdaptiveLSPServer doesn't even use Commands.Completion & Commands.HelpText any more.

-> Closed