Flaky LSP HoverRequest response

[marvinborner]

While working on the LSP testing suite (#351), I had several problems with the textDocument/hover request. I initially believed the problem to be in my LSP client implementation (and I was correct, to some extent) but now I think the problem also lies in Effekt/Kiama.

A reproducible communication flow that sometimes returns the wrong result is as follows:

1. InitializeRequest -> ...
2. InitializedNotification
3. DidOpenTextDocumentNotification effect Exc(msg: String): Unit
4. HoverRequest (e.g. line 0, char 8) -> null / { ... }

Where the last HoverRequest returns null instead of the correct result around 1/2 of the times.

You can try it yourself using this JS script.

I spent a lot of time trying to debug this but ultimately did not get much further than that this function call sometimes returns None instead of Some(...):

https://github.com/effekt-lang/effekt/blob/e0d28ef099d3004ddfdde63d1e07826c7983b265/effekt/jvm/src/main/scala/effekt/Server.scala#L91

I was not able to reproduce this issue with other similar request types.

[b-studios]

Maybe it is some kind of timing issue? It looks like process (which in turn will compile the file) will only be called onsave

https://github.com/effekt-lang/kiama/blob/3401470a4cbbe294d2df0c6923e919b8a45ca1f5/jvm/src/main/scala/kiama/util/Server.scala#L498

so maybe this call is not fully processed, when the onhover is triggered?

[b-studios]

I don't know what's the LSP best practices here. Should there be some form of synchronization? Should we block until the first process is completed? Or is this completely against how LSP usually works?

[marvinborner]

Don't we already process on textDocument/didOpen here and, in the reproduction, here?

I assumed process to be blocking already, otherwise it should probably return a future or something, right? I think this is probably the best practice, I have not come across synchronization yet.

It's also interesting that if you comment out this line, i.e. always open and close the same file, the following hover results will always stay on the first result (null/{...}).

[b-studios]

I just thought there might be a race condition with save, open, or another action that will indeed perform the processing but is not finished yet.

[marvinborner]

I tried the fix we talked about again; doesn't change anything. I believe that was the idea, right:

def getSymbolAt(position: Position)(implicit C: Context): Option[(Tree, Symbol)] =
  for {
    id <- getIdTreeAt(position)
    _ = C.compiler.Frontend.run(position.source) // hotfix
    sym <- C.symbolOption(id)
  } yield (id, resolveCallTarget(sym))

[b-studios]

Yes, exactly. This is really strange. Does adding sleep in some places help to diagnose?

[marvinborner]

Added many sleeps, doesn't change anything (except execution time)... So weird