withProgress Cancellable doesn't seem to work?

[yav]

Hello,
I've just started using this package recently, so possibly I am using it incorrectly, or misunderstand something. The issue is that I can't seem to get withProgress to work correctly when an action is cancelled. To test it, I've made a server function like this:

onExecuteCommand :: LSP.ExecuteCommandParams -> M () -> M ()
onExecuteCommand _ps k =
  Srv.withProgress "PROG" Srv.Cancellable (go 0)
  where
  go n advance =
    if n < 100
      then do
        liftIO (threadDelay (1 * 10^(6::Int)))
        _ <- advance (Srv.ProgressAmount (Just n)
                     (Just (Text.pack ("THING: " ++ show n))))
        go (n+5) advance
      else k
    `catch` \(_ :: Srv.ProgressCancelledException) -> do
      lspShow Info "Cancelled"

Basically the idea is that we sleep for a second a bunch of times to simulate work. This works great as long as you don't cancel the action. If the action is cancelled, the work seems to continue executing, and when it is finished I get this error:

LSP: no handler for: "window/workDoneProgress/cancel"

Looking at lsp's source code, it looks like there is a built-in handler for this, which makes sense. However, this handler never seems to fire.

I wonder if the problem is that in the implementation of withProgress, we fork off the work with async, but then we wait for it straight away, so we are blocked and can't handle the cancel message from the client?

Thoughts?

[michaelpj]

Yes, I think this is fundamentally broken if you're not running your handlers concurrently, like HLS does (or see the "reactor" example). I also think the progress handling is broken for this reason.

Part of the problem is that I think you can't do this without concurrent handling of requests! If you have only one thread that is executing an action, how can it possibly notice that you have received a cancel request? So in that sense a non-concurrent LSP server is just broken (or can't support some features). I think we should make concurrent the default, but I haven't yet had time to figure out how to do that nicely while still giving the user a way to control how the scheduling happens...

See also #409

[yav]

OK, I can refactor my server to run the handlers concurrently. In that case, perhaps it would make sense to change withPorgress to avoid forking another thread for the work?

[michaelpj]

I'm not sure how that would help. We have to have a way to cancel the running computation when a cancellation request comes in, killing the thread that is running it as async does is a simple way to do that.

[yav]

I am thinking that it'd be good if either (A) the user of the library is in charge of concurrency (they fork and kill), or (B) the library is in charge (i.e., the library forks and kills). The current approach where the user has to fork some handlers, and the library does another fork so it can kill things, seems confusing.

I think in #540 you are essentially suggesting design (B), which I think is a good idea, and if I understand correctly, once the library starts forking off the handlers, there would be no need to do another fork in the handler, as we currently are.

Or am I misunderstanding the situation?

[michaelpj]

I'm not really sure what to do. The thing is, it probably is reasonable for the user to want some kind of control. E.g. you might want to throttle certain kinds of request. Just forcing everyone into unbounded concurrency seems maybe bad? But maybe it's fine and I should just stop worrying about it - if you really care you can probably do it by taking a semaphore inside the handlers or something.

[wz1000]

We don't want to fork handlers in LSP because then clients of the library have no way to determine the order of messages, which is essential for correct processing. For example, in HLS we process all notifications sequentially, as we must for anything to make sense.

Perhaps we can fork a single thread to allow messages to be decoded asynchronously, basically integrating the reactor example into the library.

This doesn't suffice for cancellation, but I can think of a few ways to handle it:

1. Provide an extra MVar () argument to cancellable handlers, and rely on the handlers to wait on the signal, typically something like race_ (readMVar cancelVar) handlerCode
2. Have cancellable handlers return a ThreadId or perhaps a Maybe ThreadId and send exceptions to that thread on cancellation

[michaelpj]

Well no, we absolutely can do that, indeed what I was thinking about is to essentially port the HLS scheduler to lsp. I think pretty much everyone is going to want prompt and in-order handling of notifications (even if the spec is very vague on this front). And handling notifications in sequence doesn't mean not using async: you can spawn them just like anything else, it's just that you wait for the task immediately and don't do anything else until it's done.

Some care may be needed to handle cancellations that come in before you've started things etc. but it all seems possible to handle.

But the fact that this is complicated suggests to me that we maybe just want one scheduler that does it right. If what it does is wrong for HLS, it's probably just wrong.

[michaelpj]

Oh, right, in fact we also don't even need to fork off notification handlers, since notifications aren't cancellable.

[wz1000]

HLS currently doesn't particularly care about request order, but it might want to do so in the future. I'm pretty sure the spec explicitly states that request processing order matters.

[michaelpj]

Well, the spec gives an extremely lukewarm take on this:

However, the server may decide to use a parallel execution strategy and may wish to return responses in a different order than the requests were received. The server may do so as long as this reordering doesn’t affect the correctness of the responses.

I think there is at least the following level of necessary concurrency:

• The server must be able to process a notification that comes after a request while still processing the request, otherwise $/cancel cannot work.
  • So: notifications should be processed concurrently with requests
• The server must be able to send a request and process the response while processing a request, otherwise window/workDoneProgress/create cannot work.
  • So: handling of responses should be concurrent with requests
• The server really should be able to process additional requests while processing an initial request, since otherwise any long-running request will block all functionality.
  • So: handling of most requests should be concurrent with other requests

Yes, there are some ordering constraints that do matter (e.g. you have to process notifications in order). I think there are few others (the spec suggests that requests that cause modifications should be done in order), but I think most of those are generic, i.e. we could handle any such ordering constraints in lsp, rather than HLS.

Plus, this all sounds very fiddly and probably best to at least have a sensible default in lsp, since I bet it's hard for other users to get right!

[TimWhiting]

For reference this is how we are currently doing cancellation in Koka. (Sidenote, adding Koka's language server was my first time working in Haskell, and especially working with any asynchronous Haskell, so if there are errors in the logic, please point them out!). It was inspired by HLS's implementation, but I tried to simplify it. I also decided to discard all file change notifications except the most recent for any given file to avoid processing outdated notifications.

https://github.com/koka-lang/koka/blob/f48555f1e2ba211f3e86524a15668597cad19118/src/Main/langserver/LanguageServer/Handlers.hs#L113

Edit: this comment probably belongs in #540

[michaelpj]

Sorry this is a pain, this is next on my list of stuff to improve!

also decided to discard all file change notifications except the most recent for any given file to avoid processing outdated notifications.

If this is just for noticing that a file has changed I think that's probably safe. Generally indeed you want to debounce inputs that require you to redo work; that's one place to do it. (Personally I would probably have an internal queue of work such as "recompile this file" and then debounce that, but up to you). Also would #555 be helpful for you?