[lldb-dap] Adding support for cancelling a request. #130169
Conversation
| // }, | ||
| llvm::Expected<CancelResponseBody> | ||
| CancelRequestHandler::Run(const CancelArguments &arguments) const { | ||
| /* no-op, simple ack of the request. */ |
There was a problem hiding this comment.
I would have expected that we do something here. Or at least a comment which explains me, why we not don't have to do anything here...
This comment currently explains the "what?" (which is kind of obvious from the code). I would prefer a comment explaining the "why?" (which is much harder to reconstruct by reading the code)
There was a problem hiding this comment.
I updated the comment to explain that this is handled in the DAP::Loop instead.
ashgti
force-pushed
the
cancel-request
branch
2 times, most recently
from
6bb322f to
8b71292
Compare
|
@llvm/pr-subscribers-lldb Author: John Harrison (ashgti) ChangesAdding support for cancelling requests. There are two forms of request cancellation.
Patch is 24.20 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/130169.diff 15 Files Affected:
diff --git a/lldb/test/API/tools/lldb-dap/cancel/Makefile b/lldb/test/API/tools/lldb-dap/cancel/Makefile
new file mode 100644
index 0000000000000..10495940055b6
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/cancel/Makefile
@@ -0,0 +1,3 @@
+C_SOURCES := main.c
+
+include Makefile.rules
diff --git a/lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py b/lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py
new file mode 100644
index 0000000000000..f3b2f9fcb7a92
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/cancel/TestDAP_cancel.py
@@ -0,0 +1,101 @@
+"""
+Test lldb-dap cancel request
+"""
+
+import time
+
+from lldbsuite.test.decorators import *
+from lldbsuite.test.lldbtest import *
+import lldbdap_testcase
+
+
+class TestDAP_launch(lldbdap_testcase.DAPTestCaseBase):
+ def send_async_req(self, command: str, arguments={}) -> int:
+ seq = self.dap_server.sequence
+ self.dap_server.send_packet(
+ {
+ "type": "request",
+ "command": command,
+ "arguments": arguments,
+ }
+ )
+ return seq
+
+ def async_blocking_request(self, duration: float) -> int:
+ """
+ Sends an evaluate request that will sleep for the specified duration to
+ block the request handling thread.
+ """
+ return self.send_async_req(
+ command="evaluate",
+ arguments={
+ "expression": '`script import time; print("starting sleep", file=lldb.debugger.GetOutputFileHandle()); time.sleep({})'.format(
+ duration
+ ),
+ "context": "repl",
+ },
+ )
+
+ def async_cancel(self, requestId: int) -> int:
+ return self.send_async_req(command="cancel", arguments={"requestId": requestId})
+
+ def test_pending_request(self):
+ """
+ Tests cancelling a pending request.
+ """
+ program = self.getBuildArtifact("a.out")
+ self.build_and_launch(program, stopOnEntry=True)
+ self.continue_to_next_stop()
+
+ # Use a relatively short timeout since this is only to ensure the
+ # following request is queued.
+ blocking_seq = self.async_blocking_request(duration=1.0)
+ # Use a longer timeout to ensure we catch if the request was interrupted
+ # properly.
+ pending_seq = self.async_blocking_request(duration=self.timeoutval)
+ cancel_seq = self.async_cancel(requestId=pending_seq)
+
+ blocking_resp = self.dap_server.recv_packet(filter_type=["response"])
+ self.assertEqual(blocking_resp["request_seq"], blocking_seq)
+ self.assertEqual(blocking_resp["command"], "evaluate")
+ self.assertEqual(blocking_resp["success"], True)
+
+ pending_resp = self.dap_server.recv_packet(filter_type=["response"])
+ self.assertEqual(pending_resp["request_seq"], pending_seq)
+ self.assertEqual(pending_resp["command"], "evaluate")
+ self.assertEqual(pending_resp["success"], False)
+ self.assertEqual(pending_resp["message"], "cancelled")
+
+ cancel_resp = self.dap_server.recv_packet(filter_type=["response"])
+ self.assertEqual(cancel_resp["request_seq"], cancel_seq)
+ self.assertEqual(cancel_resp["command"], "cancel")
+ self.assertEqual(cancel_resp["success"], True)
+ self.continue_to_exit()
+
+ def test_inflight_request(self):
+ """
+ Tests cancelling an inflight request.
+ """
+ program = self.getBuildArtifact("a.out")
+ self.build_and_launch(program, stopOnEntry=True)
+ self.continue_to_next_stop()
+
+ blocking_seq = self.async_blocking_request(duration=self.timeoutval / 2)
+ # Wait for the sleep to start to cancel the inflight request.
+ self.collect_stdout(
+ timeout_secs=self.timeoutval,
+ pattern="starting sleep",
+ )
+ cancel_seq = self.async_cancel(requestId=blocking_seq)
+
+ blocking_resp = self.dap_server.recv_packet(filter_type=["response"])
+ self.assertEqual(blocking_resp["request_seq"], blocking_seq)
+ self.assertEqual(blocking_resp["command"], "evaluate")
+ self.assertEqual(blocking_resp["success"], False)
+ self.assertEqual(blocking_resp["message"], "cancelled")
+
+ cancel_resp = self.dap_server.recv_packet(filter_type=["response"])
+ self.assertEqual(cancel_resp["request_seq"], cancel_seq)
+ self.assertEqual(cancel_resp["command"], "cancel")
+ self.assertEqual(cancel_resp["success"], True)
+ self.continue_to_exit()
diff --git a/lldb/test/API/tools/lldb-dap/cancel/main.c b/lldb/test/API/tools/lldb-dap/cancel/main.c
new file mode 100644
index 0000000000000..ecc0d99ec8db7
--- /dev/null
+++ b/lldb/test/API/tools/lldb-dap/cancel/main.c
@@ -0,0 +1,6 @@
+#include <stdio.h>
+
+int main(int argc, char const *argv[]) {
+ printf("Hello world!\n");
+ return 0;
+}
diff --git a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
index 0c92e5bff07c6..5fb088dc51cbb 100644
--- a/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
+++ b/lldb/test/API/tools/lldb-dap/launch/TestDAP_launch.py
@@ -27,6 +27,7 @@ def test_default(self):
lines = output.splitlines()
self.assertIn(program, lines[0], "make sure program path is in first argument")
+ @skipIfWindows
def test_termination(self):
"""
Tests the correct termination of lldb-dap upon a 'disconnect'
diff --git a/lldb/tools/lldb-dap/CMakeLists.txt b/lldb/tools/lldb-dap/CMakeLists.txt
index adad75a79fa7a..1e87b28c6d1bd 100644
--- a/lldb/tools/lldb-dap/CMakeLists.txt
+++ b/lldb/tools/lldb-dap/CMakeLists.txt
@@ -40,6 +40,7 @@ add_lldb_tool(lldb-dap
Handler/ResponseHandler.cpp
Handler/AttachRequestHandler.cpp
Handler/BreakpointLocationsHandler.cpp
+ Handler/CancelRequestHandler.cpp
Handler/CompileUnitsRequestHandler.cpp
Handler/CompletionsHandler.cpp
Handler/ConfigurationDoneRequestHandler.cpp
diff --git a/lldb/tools/lldb-dap/DAP.cpp b/lldb/tools/lldb-dap/DAP.cpp
index a1e2187288768..ebdaa4949eb77 100644
--- a/lldb/tools/lldb-dap/DAP.cpp
+++ b/lldb/tools/lldb-dap/DAP.cpp
@@ -39,9 +39,11 @@
#include <algorithm>
#include <cassert>
#include <chrono>
+#include <condition_variable>
#include <cstdarg>
#include <cstdio>
#include <fstream>
+#include <future>
#include <memory>
#include <mutex>
#include <string>
@@ -241,6 +243,21 @@ void DAP::SendJSON(const llvm::json::Value &json) {
}
void DAP::Send(const protocol::Message &message) {
+ if (auto *resp = std::get_if<protocol::Response>(&message);
+ resp && debugger.InterruptRequested()) {
+ // If the debugger was interrupted, convert this response into a 'cancelled'
+ // response.
+ protocol::Response cancelled;
+ cancelled.command = resp->command;
+ cancelled.request_seq = resp->request_seq;
+ cancelled.success = false;
+ cancelled.message = protocol::Response::Message::cancelled;
+ if (llvm::Error err = transport.Write(cancelled))
+ DAP_LOG_ERROR(log, std::move(err), "({1}) write failed: {0}",
+ transport.GetClientName());
+ return;
+ }
+
if (llvm::Error err = transport.Write(message))
DAP_LOG_ERROR(log, std::move(err), "({1}) write failed: {0}",
transport.GetClientName());
@@ -674,6 +691,10 @@ void DAP::SetTarget(const lldb::SBTarget target) {
bool DAP::HandleObject(const protocol::Message &M) {
if (const auto *req = std::get_if<protocol::Request>(&M)) {
+ // Clear interrupt marker prior to handling the next request.
+ if (debugger.InterruptRequested())
+ debugger.CancelInterruptRequest();
+
auto handler_pos = request_handlers.find(req->command);
if (handler_pos != request_handlers.end()) {
(*handler_pos->second)(*req);
@@ -778,28 +799,134 @@ llvm::Error DAP::Disconnect(bool terminateDebuggee) {
return ToError(error);
}
+template <typename T>
+static std::optional<T> getArgumentsIfRequest(const protocol::Message &pm,
+ llvm::StringLiteral command) {
+ auto *const req = std::get_if<protocol::Request>(&pm);
+ if (!req || req->command != command)
+ return std::nullopt;
+
+ T args;
+ llvm::json::Path::Root root;
+ if (!fromJSON(req->arguments, args, root)) {
+ return std::nullopt;
+ }
+
+ return std::move(args);
+}
+
llvm::Error DAP::Loop() {
- auto cleanup = llvm::make_scope_exit([this]() {
+ std::deque<protocol::Message> queue;
+ std::condition_variable queue_cv;
+ std::mutex queue_mutex;
+ std::future<llvm::Error> queue_reader = std::async([&]() -> llvm::Error {
+ llvm::set_thread_name(transport.GetClientName() + ".transport_handler");
+ auto cleanup = llvm::make_scope_exit([&]() {
+ // Ensure we're marked as disconnecting when the reader exits.
+ disconnecting = true;
+ queue_cv.notify_all();
+ });
+
+ while (!disconnecting) {
+ llvm::Expected<std::optional<protocol::Message>> next =
+ transport.Read(std::chrono::seconds(1));
+ bool timeout = false;
+ if (llvm::Error Err = llvm::handleErrors(
+ next.takeError(),
+ [&](std::unique_ptr<llvm::StringError> Err) -> llvm::Error {
+ if (Err->convertToErrorCode() == std::errc::timed_out) {
+ timeout = true;
+ return llvm::Error::success();
+ }
+ return llvm::Error(std::move(Err));
+ }))
+ return Err;
+
+ // If the read timed out, continue to check if we should disconnect.
+ if (timeout)
+ continue;
+
+ // nullopt is returned on EOF.
+ if (!*next)
+ break;
+
+ {
+ std::lock_guard<std::mutex> lock(queue_mutex);
+
+ // If a cancel is requested for the active request, make a best
+ // effort attempt to interrupt.
+ if (const auto cancel_args =
+ getArgumentsIfRequest<protocol::CancelArguments>(**next,
+ "cancel");
+ cancel_args && active_seq == cancel_args->requestId) {
+ DAP_LOG(log, "({0}) interrupting inflight request {1}",
+ transport.GetClientName(), active_seq);
+ debugger.RequestInterrupt();
+ debugger.GetCommandInterpreter().InterruptCommand();
+ }
+
+ queue.push_back(std::move(**next));
+ }
+ queue_cv.notify_one();
+ }
+
+ return llvm::Error::success();
+ });
+
+ auto cleanup = llvm::make_scope_exit([&]() {
out.Stop();
err.Stop();
StopEventHandlers();
});
+
while (!disconnecting) {
- llvm::Expected<std::optional<protocol::Message>> next = transport.Read();
- if (!next)
- return next.takeError();
+ protocol::Message next;
+ {
+ std::unique_lock<std::mutex> lock(queue_mutex);
+ queue_cv.wait(lock, [&] { return disconnecting || !queue.empty(); });
- // nullopt on EOF
- if (!*next)
- break;
+ if (queue.empty())
+ break;
+
+ next = queue.front();
+ queue.pop_front();
+
+ if (protocol::Request *req = std::get_if<protocol::Request>(&next)) {
+ active_seq = req->seq;
+
+ // Check if we should preempt this request from a queued cancel.
+ bool cancelled = false;
+ for (const auto &message : queue) {
+ if (const auto args =
+ getArgumentsIfRequest<protocol::CancelArguments>(message,
+ "cancel");
+ args && args->requestId == req->seq) {
+ cancelled = true;
+ break;
+ }
+ }
- if (!HandleObject(**next)) {
+ // Preempt the request and immeidately respond with cancelled.
+ if (cancelled) {
+ protocol::Response response;
+ response.request_seq = req->seq;
+ response.command = req->command;
+ response.success = false;
+ response.message = protocol::Response::Message::cancelled;
+ Send(response);
+ continue;
+ }
+ } else
+ active_seq = 0;
+ }
+
+ if (!HandleObject(next)) {
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"unhandled packet");
}
}
- return llvm::Error::success();
+ return queue_reader.get();
}
lldb::SBError DAP::WaitForProcessToStop(uint32_t seconds) {
diff --git a/lldb/tools/lldb-dap/DAP.h b/lldb/tools/lldb-dap/DAP.h
index 4c57f9fef3d89..17c5ec2bfee9f 100644
--- a/lldb/tools/lldb-dap/DAP.h
+++ b/lldb/tools/lldb-dap/DAP.h
@@ -395,6 +395,9 @@ struct DAP {
InstructionBreakpoint *GetInstructionBreakpoint(const lldb::break_id_t bp_id);
InstructionBreakpoint *GetInstructionBPFromStopReason(lldb::SBThread &thread);
+
+private:
+ std::atomic<int64_t> active_seq;
};
} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp
new file mode 100644
index 0000000000000..e6746316e6e74
--- /dev/null
+++ b/lldb/tools/lldb-dap/Handler/CancelRequestHandler.cpp
@@ -0,0 +1,55 @@
+//===-- CancelRequestHandler.cpp ------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "Handler/RequestHandler.h"
+#include "Protocol/ProtocolRequests.h"
+#include "llvm/Support/Error.h"
+
+using namespace lldb_dap;
+using namespace lldb_dap::protocol;
+
+namespace lldb_dap {
+
+/// The `cancel` request is used by the client in two situations:
+///
+/// - to indicate that it is no longer interested in the result produced by a
+/// specific request issued earlier
+/// - to cancel a progress sequence.
+///
+/// Clients should only call this request if the corresponding capability
+/// `supportsCancelRequest` is true.
+///
+/// This request has a hint characteristic: a debug adapter can only be
+/// expected to make a 'best effort' in honoring this request but there are no
+/// guarantees.
+///
+/// The `cancel` request may return an error if it could not cancel
+/// an operation but a client should refrain from presenting this error to end
+/// users.
+///
+/// The request that got cancelled still needs to send a response back.
+/// This can either be a normal result (`success` attribute true) or an error
+/// response (`success` attribute false and the `message` set to `cancelled`).
+///
+/// Returning partial results from a cancelled request is possible but please
+/// note that a client has no generic way for detecting that a response is
+/// partial or not.
+///
+/// The progress that got cancelled still needs to send a `progressEnd` event
+/// back.
+///
+/// A client should not assume that progress just got cancelled after sending
+/// the `cancel` request.
+llvm::Expected<CancelResponseBody>
+CancelRequestHandler::Run(const CancelArguments &arguments) const {
+ // Cancel support is built into the DAP::Loop handler for detecting
+ // cancellations of pending or inflight requests.
+ return CancelResponseBody();
+}
+
+} // namespace lldb_dap
diff --git a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
index 3262b70042a0e..4373c59798b75 100644
--- a/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
+++ b/lldb/tools/lldb-dap/Handler/InitializeRequestHandler.cpp
@@ -463,6 +463,8 @@ void InitializeRequestHandler::operator()(
body.try_emplace("supportsDataBreakpoints", true);
// The debug adapter supports the `readMemory` request.
body.try_emplace("supportsReadMemoryRequest", true);
+ // The debug adapter supports the `cancel` request.
+ body.try_emplace("supportsCancelRequest", true);
// Put in non-DAP specification lldb specific information.
llvm::json::Object lldb_json;
diff --git a/lldb/tools/lldb-dap/Handler/RequestHandler.h b/lldb/tools/lldb-dap/Handler/RequestHandler.h
index c9bcf15933c33..32c3199f4904b 100644
--- a/lldb/tools/lldb-dap/Handler/RequestHandler.h
+++ b/lldb/tools/lldb-dap/Handler/RequestHandler.h
@@ -381,6 +381,16 @@ class ReadMemoryRequestHandler : public LegacyRequestHandler {
void operator()(const llvm::json::Object &request) const override;
};
+class CancelRequestHandler
+ : public RequestHandler<protocol::CancelArguments,
+ protocol::CancelResponseBody> {
+public:
+ using RequestHandler::RequestHandler;
+ static llvm::StringLiteral getCommand() { return "cancel"; }
+ llvm::Expected<protocol::CancelResponseBody>
+ Run(const protocol::CancelArguments &args) const override;
+};
+
/// A request used in testing to get the details on all breakpoints that are
/// currently set in the target. This helps us to test "setBreakpoints" and
/// "setFunctionBreakpoints" requests to verify we have the correct set of
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
index 5cc5429227439..9b36c8329b0fc 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.cpp
@@ -15,6 +15,13 @@ using namespace llvm;
namespace lldb_dap::protocol {
+bool fromJSON(const llvm::json::Value &Params, CancelArguments &CA,
+ llvm::json::Path P) {
+ llvm::json::ObjectMapper O(Params, P);
+ return O && O.mapOptional("requestId", CA.requestId) &&
+ O.mapOptional("progressId", CA.progressId);
+}
+
bool fromJSON(const json::Value &Params, DisconnectArguments &DA,
json::Path P) {
json::ObjectMapper O(Params, P);
diff --git a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
index 5dc4a589178d2..8acdcd322f526 100644
--- a/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
+++ b/lldb/tools/lldb-dap/Protocol/ProtocolRequests.h
@@ -29,6 +29,26 @@
namespace lldb_dap::protocol {
+/// Arguments for `cancel` request.
+struct CancelArguments {
+ /// The ID (attribute `seq`) of the request to cancel. If missing no request
+ /// is cancelled.
+ ///
+ /// Both a `requestId` and a `progressId` can be specified in one request.
+ std::optional<int64_t> requestId;
+
+ /// The ID (attribute `progressId`) of the progress to cancel. If missing no
+ /// progress is cancelled.
+ ///
+ /// Both a `requestId` and a `progressId` can be specified in one request.
+ std::optional<int64_t> progressId;
+};
+bool fromJSON(const llvm::json::Value &, CancelArguments &, llvm::json::Path);
+
+/// Response to `cancel` request. This is just an acknowledgement, so no body
+/// field is required.
+using CancelResponseBody = VoidResponse;
+
/// Arguments for `disconnect` request.
struct DisconnectArguments {
/// A value of true indicates that this `disconnect` request is part of a
diff --git a/lldb/tools/lldb-dap/Transport.cpp b/lldb/tools/lldb-dap/Transport.cpp
index 4500e7cf909ba..2241ea586c19a 100644
--- a/lldb/tools/lldb-dap/Transport.cpp
+++ b/lldb/tools/lldb-dap/Transport.cpp
@@ -10,6 +10,7 @@
#include "DAPLog.h"
#include "Protocol/ProtocolBase.h"
#include "lldb/Utility/IOObject.h"
+#include "lldb/Utility/SelectHelper.h"
#include "lldb/Utility/Status.h"
#include "lldb/lldb-forward.h"
#include "llvm/ADT/StringExtras.h"
@@ -27,23 +28,39 @@ using namespace lldb_dap::protocol;
/// ReadFull attempts to read the specified number of bytes. If EOF is
/// encountered, an empty string is returned.
-static Expected<std::string> ReadFull(IOObject &descriptor, size_t length) {
+static Expected<std::string>
+ReadFull(IOObject &descriptor, size_t length,
+ const std::chrono::microseconds &timeout) {
+ if (!descriptor.IsValid())
+ return createStringError("transport output is closed");
+
+#ifndef _WIN32
+ // FIXME: SelectHelper does not work with NativeFile on Win32.
+ SelectHelper sh;
+ sh.SetTimeout(timeout);
+ sh.FDSetRead(descriptor.GetWaitableHandle());
+ Status status = sh.Select();
+ if (status.Fail())
+ return status.takeError();
+#endif
+
std::string data;
data.resize(length);
- auto status = descript...
[truncated]
|
| @@ -27,6 +27,7 @@ def test_default(self): | |||
| lines = output.splitlines() | |||
| self.assertIn(program, lines[0], "make sure program path is in first argument") | |||
|
|
|||
| @skipIfWindows | |||
There was a problem hiding this comment.
why? Did this test case work previously and was now broken by implementing cancellation support?
| // The debug adapter supports the `cancel` request. | ||
| body.try_emplace("supportsCancelRequest", true); |
There was a problem hiding this comment.
move to CancelRequestHandler (#131943)
lldb/tools/lldb-dap/Transport.cpp
Outdated
|
|
||
| Expected<std::string> raw_json = ReadFull(*input, length); |
There was a problem hiding this comment.
we should probably only apply a timeout before receiving the first byte of a message.
Otherwise, we might run into hard-to-debug issues where the client sends
Content-Length: 123
\r\n\r\n
<wait for 2 seconds>
actual request body
With the current logic, we would first consume the Content-Length: 123\r\n\rn\n header, then run into the timeout. Upon retrying the read in DAP::Loop() we would the find the request body without the required Content-Length header.
The client would be compliant with the Debug Adapter Protocol specification, yet lldb-dap would choke on this message.
It seems we are only using the timeout such that the disconnecting flag is checked regularly in DAP::Loop. Instead of using a timeout to wake up the reader-thread, would it maybe make sense to instead call Transport::Close when we want to shut down the reader? That should also cancel any outstanding reads, doesn't it?
There was a problem hiding this comment.
we should probably only apply a timeout before receiving the first byte of a message.
Otherwise, we might run into hard-to-debug issues where the client sends
Yea, thats a good idea, I'll only apply the timeout to the message header.
It seems we are only using the timeout such that the
disconnectingflag is checked regularly inDAP::Loop. Instead of using a timeout to wake up the reader-thread, would it maybe make sense to instead callTransport::Closewhen we want to shut down the reader? That should also cancel any outstanding reads, doesn't it?
Closing a file descriptor does not interrupt in progress reads, I think its platform dependent on what happens.
There was a problem hiding this comment.
Also, frustrating on Windows stdin is an anonymous pipe and anonymous pipes do not support async reads (https://learn.microsoft.com/en-us/windows/win32/ipc/anonymous-pipe-operations) which is why I disabled the one test on Windows, since we can't read with a timeout on stdin. Which does mean we'd end up blocking the full shutdown of lldb-dap on VSCode closing the input side of the pipe after it calls 'disconnect', but that should be fine, IMO.
There was a problem hiding this comment.
Closing a file descriptor does not interrupt in progress reads, I think its platform dependent on what happens.
Very much so. It's also basically impossible to do this in race-free manner,.
Also, frustrating on Windows stdin is an anonymous pipe and anonymous pipes do not support
I don't know if this helps you (since you still have to use a dedicated API instead of select(2)), but lldb_private::Pipe works around this by creating a named pipe with an random name (from what I've read, that's actually how windows implements anonymous pipes internally).
There was a problem hiding this comment.
I don't know if this helps you (since you still have to use a dedicated API instead of
select(2)), butlldb_private::Pipeworks around this by creating a named pipe with an random name (from what I've read, that's actually how windows implements anonymous pipes internally).
The trouble is that when we run the debugger as an DebugAdapterExecutable the input stream is stdin. Unless we want to make Windows always use a local socket DebugAdapterServer or named pipes DebugAdapterNamedPipeServer we'd need to have some other mechanism for supporting reading with a timeout on Windows.
But I'm also not sure if that actually matters. VSCode never closes stdin, so it never gets an EOF. When the debug session ends, it sends a kill -15 https://github.com/microsoft/vscode/blob/main/src/vs/workbench/contrib/debug/node/debugAdapter.ts#L290-L303 so I'm not sure if we need to ensure the Transport returns, but we do have an existing test that we shutdown after the disconnect request is sent.
Adding support for cancelling requests. There are two forms of request cancellation. * Preemptively cancelling a request that is in the queue. * Actively cancelling the in progress request as a best effort attempt using `SBDebugger.RequestInterrupt()`.
…der, once we have the header we should read the full message.
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I left a few small comments but overall this looks good. The approach is slightly different from how I expected you to implement this. Note that I don't know if it's actually better, it's more of a "if I had to implement this, that's how I would have started." You've been working on this for a bit, so I'm trusting your judgement. 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: If we go with the new object ( Anyway, like I said, not sure if this is actually better, just sharing my thoughts. |
There was a problem hiding this comment.
I think there's a race in the cancellation handling:
- request handling thread sets active_seq (but doesn't start executing the request yet)
- cancellation request comes in, queue_reader sets the interrupt flag
- request handling thread begins handling the request, starts by clearing the flag
- request is not cancelled
I think this would be easier to guarantee correctness (and review) if active_seq was not an atomic and its modification, as well as the acts of setting and clearing the interrupt flag were done under a mutex
| /// specific request issued earlier | ||
| /// - to cancel a progress sequence. | ||
| /// | ||
| /// Clients should only call this request if the corresponding capability |
There was a problem hiding this comment.
The documentation contains some wording with recommendations (i.e. "should"). Do we expect clients to read this? Is this coming from the DAP documentation? Otherwise it seems like this should be aimed at us, server implementers.
There was a problem hiding this comment.
Its the DAP documentation for the cancel request, its mostly verbatim with small formatting tweaks. See https://microsoft.github.io/debug-adapter-protocol/specification#Base_Protocol_Cancel
lldb/tools/lldb-dap/DAP.cpp
Outdated
| // If the debugger was interrupted, convert this response into a 'cancelled' | ||
| // response. |
There was a problem hiding this comment.
| // If the debugger was interrupted, convert this response into a 'cancelled' | |
| // response. | |
| // If the debugger was interrupted, convert this response into a 'cancelled' | |
| // response because we might have a partial result. |
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 That may not be as idiomatic in c++ though. I do like the more simplified I'll try to see if I can come up with a |
Totally, and I think that's a great design, and not something that would change with my suggestion, right?
If we want to support cancellation before moving all the request over to the protocol class, can we temporarily store both the JSON and the Protocol in the class we instantiate per request (whether that's the RequestHandler itself, or a class wrapping it) and use whichever one we need for that given request? |
For the llvm-project/lldb/tools/lldb-dap/DAP.cpp Lines 245 to 258 in 1c2d19b |
Switched to a mutex for tracking the active request. |
lldb/tools/lldb-dap/DAP.cpp
Outdated
| // Clear interrupt marker prior to handling the next request. | ||
| if (debugger.InterruptRequested()) | ||
| debugger.CancelInterruptRequest(); |
There was a problem hiding this comment.
You still have the same race here. The request could be cancelled between this line and when the active request field is set on line 702. (My complaint wasn't so much about the exact std::atomic type as it was about treating this flag as an atomic -- a mutex surrounding just a variable assignment is morally equivalent to an atomic). For this to work, I think the flag needs to be checked in the same critical section that handles sets m_active_request. I believe it will be fine if you move this code to line 702, just before setting the active request.
There was a problem hiding this comment.
Moved this up to 702
|
@JDevlieghere I moved some of the cancel checking logic into the BaseRequestHandler to try to consolidate things. There are some FIXME's around cleaning it up once all the requests have moved off the LegacyRequestHandler. |
|
Anyone have any other comments on supporting 'cancel'? |
|
Wanted to also double check with @labath as well, any additional thoughts? |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/197/builds/3983 Here is the relevant piece of the build log for the reference |
|
PR #135638 should fix the build failure. |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/18/builds/14470 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/7879 Here is the relevant piece of the build log for the reference |
Adding support for cancelling requests. There are two forms of request cancellation. * Preemptively cancelling a request that is in the queue. * Actively cancelling the in progress request as a best effort attempt using `SBDebugger.RequestInterrupt()`.
Adding support for cancelling requests.
There are two forms of request cancellation.
SBDebugger.RequestInterrupt().