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
608 stars 144 forks source link

Debug Adapter Protocol: Sending events after a response #229

Open fwcd opened 6 years ago

fwcd commented 6 years ago

The specification requires the debug adapter to send an initialized event back to the client after the initialize response has been returned:

image

AFAIK, this is not possible using the IDebugProtocolServer interface without hacking deep into the JSON-RPC machinery. Is there a workaround for this?

fwcd commented 6 years ago

A thread is probably the easiest solution, but I would not want to cause any race conditions with clients that rely on the fact that initialized happens strictly after the capabilities response.

jonahgraham commented 6 years ago

You are correct that it is bad for the adapter to send the event back first, from the spec:

A debug adapter is expected to send this [Initialized] event when it is ready to accept configuration requests (but not before the ‘initialize’ request has finished).

Frustratingly, for a long time the Mock Debug Adapter for VSCode did not respect this. Mock debug was the starting point for many implementations of debug adapters. The rest of that thread tries to clarify some of the details of how vscode operate, with a better picture of that is going on here. As you can see VS Code sends requests concurrently to the DAP.


A thread is probably the easiest solution, but I would not want to cause any race conditions with clients that rely on the fact that initialized happens strictly after the capabilities response.

I simply cannot see a way to do this with no possibility of a race condition based on current implementation of RemoteEndpoint. Strictly the problem isn't sending the initialized before the initialize response, but rather the problem is sending the launch response before the configuration done response because you can delay sending initialized until you receive the launch request.

This is what almost works, but it can theoretically run the thread defined in configurationDone() before configurationDone() returns, meaning that the launch reply would go before the configurationDone reply. For the current VSCode this is not a problem AFAICT.

    // A thread pool executor
    ExecutorService executor;
    IDebugProtocolClient client;
    CompletableFuture<Void> launchFuture;

    @Override
    public CompletableFuture<Capabilities> initialize(InitializeRequestArguments args) {
        Capabilities capabilities = new Capabilities();
        capabilities.setSupportsConfigurationDoneRequest(true);
        // ... set other capabilities here
        return CompletableFuture.completedFuture(capabilities);
    }

    @Override
    public CompletableFuture<Void> launch(Map<String, Object> args) {
        client.initialized();

        // start debugger here, but don't complete it's launch until we get config done
        launchFuture = new CompletableFuture<>();
        return launchFuture;
    }

    @Override
    public CompletableFuture<SetBreakpointsResponse> setBreakpoints(SetBreakpointsArguments args) {
        // set breakpoints in debugger
        SetBreakpointsResponse setBreakpointsResponse = new SetBreakpointsResponse();
        // fill in details of setBreakpointsResponse
        return CompletableFuture.completedFuture(setBreakpointsResponse);
    }

    @Override
    public CompletableFuture<Void> configurationDone(ConfigurationDoneArguments args) {
        CompletableFuture<Void> configurationDone = new CompletableFuture<>();
        executor.execute(() -> {
            configurationDone.complete(null);
            launchFuture.complete(null);
        });
        return configurationDone;
    }

As a temporary (bad) workaround until we solve properly you can change configurationDone method to make sure that it has fully returned like this, as long as your executor service guarantees not to run runnable within the calling thread!

    @Override
    public CompletableFuture<Void> configurationDone(ConfigurationDoneArguments args) {
        CompletableFuture<Void> configurationDone = new CompletableFuture<>();
        executor.execute(() -> {
            while (configurationDone.getNumberOfDependents() == 0) {
                // wait to make sure return of configurationDone() method is done and
                // the dependent has been added before marking configurationDone as complete
                // to ensure in order operation.
            }
            configurationDone.complete(null);
            launchFuture.complete(null);
        });
        return configurationDone;
    }

So, now on to the (proper) solution. It is possible to customize DebugRemoteEndpoint to allow you to hook into after RemoteEndpoint.handleRequest(RequestMessage) runs to ensure order of operations. I don't know how this looks at the moment, so I will get back to you on this.

jonahgraham commented 6 years ago

PS. In VSCode, but not in the spec, what happens when client receives configurationDone is to send a threads request. So the other temporary workaround is to complete launchFuture at the top of threads request.

fwcd commented 6 years ago

@jonahgraham Thank you, I will try the first approach until a clean solution is integrated.

FeldrinH commented 1 year ago

Has anything changed? Is there now a cleaner solution to this problem integrated into lsp4j?

FeldrinH commented 1 year ago

It would be really nice if there was built-in support for this in lsp4j.

As far as I can tell the suggestion of hooking into RemoteEndpoint.handleRequest(RequestMessage) can only be implemented inside lsp4j, because handleRequest does not expose the future that you need to hook and replacing the handleRequest implementation in a subclass of RemoteEndpoint can't be done without a lot of dirty hacks, because it uses a private field in RemoteEndpoint (receivedRequestMap).

jonahgraham commented 1 year ago

Has anything changed? Is there now a cleaner solution to this problem integrated into lsp4j?

Nothing has changed - but from your recent comment you seem to have some concrete ideas on how to improve it. Can you turn that into a PR?

FeldrinH commented 1 year ago

I have a usable but somewhat dirty workaround that hooks into MessageConsumer.consume instead implemented for https://github.com/goblint/GobPie/pull/60. I have a workable idea for a clean technical implementation, but I don't have a good idea for the public API. I'll look into turning it into a PR when I have time (possibly sometime next week).