[Lldb-commits] [lldb] [lldb-dap] Adding support for cancelling a request. (PR #130169)

Thu Mar 20 12:50:54 PDT 2025

ashgti wrote:

> Anyway, what I had in mind when I read your RFC and was doing the RequestHandler work, triggered by a question from @labath, is that instead of having one RequestHandler instance per command (which is what we have today), we could instantiate a RequestHandler per incoming command (or a new object that has a reference to the request hander and stores the arguments).
> 
> The read thread could decode its arguments and store away the protocol in the RequestHandler/new object and add it to the queue. Beyond that, things work pretty much how you've implemented them here, but with the logic that checks if the request has been cancelled in the RequestHandler. More concretely the operator doesn't take arguments anymore, and it's implemented something like this:
> 
> ```
> void operator() {
>   if (IsCancelled()) // Check if our request ID is in the cancelled request set.
>     RespondCancelled();
>   
>   llvm::Expected<Body> body = Run(); 
>   if (!body) 
>     // Handle error
> 
>   if (IsInterrupted()) // Check if the SBDebugger was interrupted. 
>   	RespondCancelled(); 
> }
> ```
> 
> If we go with the new object (`PendingRequest`?) the request handlers can remain what they are today, or if we want to store the data in them we'll need to find a way to dispatch to them (similar to my comment in yesterday's PR).
> 
> Anyway, like I said, not sure if this is actually better, just sharing my thoughts.

I do think this is an interesting approach. I can take a look at this as trying to wrap some of this into some helpers or maybe making the RequestHandler allocate a new instance per request.

My logic behind the `lldb_dap::protocol` types was to keep them as POD data and to not have much (if any) logic in them so that if we need to move the data between threads or queues we can be relatively confident that we're not dealing data races. In swift terms (I'm more familiar with swift than c++), the `lldb_dap::protocol` types would be `struct`'s that are all `Sendable`.

That may not be as idiomatic in c++ though.

I do like the more simplified `operator()` idea to help deal with the cancelled / interrupted, my existing solution was done to support the LegacyRequestHandlers since they're sending the responses as raw `json::Value`'s still, but with the more unified approach we could handle the cancellation in the `operator()` body.

I'll try to see if I can come up with a `PendingRequest` or maybe take a look at allocating the `RequestHandler`'s in the queue.

https://github.com/llvm/llvm-project/pull/130169