[lldb-dap] Adjusting startup flow to better handle async operations.

[ashgti]

This introduces an AsyncRequestHandler and partially reverse the changes from ba29e60 in that the launch and attach commands now run when we receive them. We wait to send the reply until we get the configurationDone request.

[ashgti]

@JDevlieghere @da-viper I think this may work to resolve #140154 and the underlying issue I saw in #140142 without needing to change our broadcaster. Leaving as a draft for now, but all the tests are passing with this change.

[da-viper]

@ashgti please could you rebase the branch as there is some risv commit that failed on the CI, will test it over the weekend.

[JDevlieghere]

I haven't looked at the code yet. I was re-reading the launch sequence part of the spec [1] to make sure this still complies, and I believe it does because the spec talks about responding to the request, not about handling it.

After the response to configurationDone is sent, the debug adapter may respond to the launch or attach request, and then the debug session has started.

That said, does this actually solve the underlying problems? We know that VS Code sends the launch and attach request before configuration done. It's equally valid to not do that, and do things the way they're shown in the sequence diagram, where you set breakpoints, then configuration done, then launch or attach. Won't that still hit the same problem?

[1] https://microsoft.github.io/debug-adapter-protocol/overview

[ashgti]

That said, does this actually solve the underlying problems? We know that VS Code sends the launch and attach request before configuration done. It's equally valid to not do that, and do things the way they're shown in the sequence diagram, where you set breakpoints, then configuration done, then launch or attach. Won't that still hit the same problem?

I think there are 2 specific problems that we have after ba29e60.

1. The pending request filter is somewhat fragile and for @da-viper use case of implementing support for supportsDataBreakpointBytes they're running into issues trying to respond to the dataBreakpointInfo request without access to a valid target but by not responding to the request, we're not getting configurationDone.
2. With the new launch flow, we're setting breakpoints before we've created a target, which is why I'm seeing us missing breakpoint events now when we used to not miss with launchCommands.

As for the sequence, the flow has 2 parallel tracks that are independently triggered. The overall flow is:

• After capabilities are returned from the request initialize then trigger the request launch or request attach
• After adapter sends event initialized, trigger the setBreakpoints, setFunctionBreakpoints, setExceptionBreakpoints, setDataBreakpoints, etc. followed by configurationDone.

We're actually triggering the configuration flow by sending event initialized so we can adjust when that happens. Per the spec:

the debug adapter is expected to send an initialized event to the development tool to announce that it is ready to accept configuration requests. With this approach a debug adapter does not have to implement a buffering strategy for configuration information.

Maybe we should look at moving the initialized event to after we've responded to the launch/attach commands instead of buffering or making them async.

[JDevlieghere]

Thanks for the summary and especially calling out that the launch and the configuration tracks are happening in parallel, I hadn't realized that until now. That explains what the "PAR" stands for in the sequence diagram... Things make a lot more sense now :-)

Alright, yeah in that case I see how we can avoid both problems by handling the launch and attach, halting the process, doing the configuration and then sending the response.

[JDevlieghere]

One more thing that comes to mind: how confident are we that all existing clients send the launch/attach in parallel with the configuration requests? If we implement what you suggest, we risk a potential deadlock when a client waits for the launch/attach to complete before sending configuration requests or inversely, if the client waits for the initialized event before sending the launch/attach.

Edit: writing this also made me realize that we assume all clients send a configurationDone request, but it's a capability they must support. I've filed #140318 to track that improvement.

[JDevlieghere]

I also found this diagram which covers both cases I had in mind: microsoft/vscode#4902 (comment)

[JDevlieghere]

I took a look at the code and I think the approach looks good.

I wonder if there's an opportunity to make this slightly more generic to avoid (1) having to change the interface (i.e. keep returning an llvm::Error) and (2) defer the error response so there's no divergence between when we send a success and failure response. That would require doing the checking of the async condition (for async requests) as well as executing the callback (if we don't yet have an error) at the layer above it. I don't know how much that complicates things or if that's worth it (do we expect to use this anywhere else?) in which case feel free to ignore this.

[ashgti]

I think #140331 is a cleaner way to solve this without the need for an AsyncRequestHandler. We may still want the AsyncRequestHandler in the future, but at least for now, I think we can avoid its added complexity.