eclipse-lsp4j / lsp4j

A Java implementation of the language server protocol intended to be consumed by tools and language servers implemented in Java.
https://eclipse.org/lsp4j
Other
609 stars 144 forks source link

Server side LSP request hangs if executed from a notification handler #775

Closed MiklosMagyari closed 9 months ago

MiklosMagyari commented 9 months ago

I don't know if it is by design, but at least it was not trivial for me and took several hours to track down.

We have a language server built on lsp4j. Among many others, we implemented the handler for didChangeWorkspaceFolders. After handling the folder changes, we call the function performing a project build that creates a work progress:

@Override
public void didChangeWorkspaceFolders(DidChangeWorkspaceFoldersParams params) {
    ...
    build();  // at a point it sends work done progress create
    ...
}

It turned out that calling the build like this causes sending window/workDoneProgress/create request to hang. Checking the trace in vscode showed that the request has been sent out and vscode responded almost immediatelly, but the CompletableFuture returned by createProgress() did not complete.

Finally, I have changed the code to:

@Override
public void didChangeWorkspaceFolders(DidChangeWorkspaceFoldersParams params) {
    ...
    CompletableFuture.runAsync(() -> {
        build();
    });
    ...
}

This way I have no lockup.

Is it intentional that I practically need to exit notification handlers before sending out an LSP request?

pisv commented 9 months ago

@MiklosMagyari Thank you for the detailed report.

Basically, LSP4J runs a loop in a dedicated thread that reads messages from an input stream and dispatches them to the corresponding handlers (for details, see StreamMessageProducer). Therefore, it is generally not a good idea to implement the handlers for requests or notifications in a blocking, synchronous way.

In your specific case, the deadlock occurred because the thread that reads and dispatches incoming messages was blocked by synchronously waiting for the response to the window/workDoneProgress/create request and, hence, was not able to read that response in the first place.

HTH

MiklosMagyari commented 9 months ago

Thanks a lot for the clarification, that explains why this lockup happened.

Btw, is this documented somewhere? Maybe I have missed it? I believe this information would be useful for others as well.

pisv commented 9 months ago

@MiklosMagyari I have opened PR #777 to update the documentation.

Any comments are welcome!

MiklosMagyari commented 9 months ago

Great! Short and concise, can spare hours of debugging. Thanks once again.

pisv commented 9 months ago

@MiklosMagyari Thanks a lot for your feedback!

I have merged the PR.