From 55980c88d4754460f200c3bcf229b7b8dfa42948 Mon Sep 17 00:00:00 2001 From: Tom Tromey Date: Wed, 20 Nov 2024 13:29:39 -0700 Subject: Allow cancellation of DAP-thread requests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This patch started as an attempt to fix the comment in CancellationHandler.cancel, but while working on it I found that the code could be improved as well. The current DAP cancellation code only handles the case where work is done on the gdb thread -- by checking for cancellation in interruptable_region. This means that if a request is handled completely in tthe DAP thread, then cancellation will never work. Now, this isn't a bug per se. DAP doesn't actually require that cancellation succeed. In fact, I think it can't, because cancellation is inherently racy. However, a coming patch will add a sort of "pending" request, and it would be nice if that were cancellable before any commands are sent to the gdb thread. No test in this patch, but one will arrive at the end of the series. Reviewed-by: Kévin Le Gouguec --- gdb/python/lib/gdb/dap/server.py | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/gdb/python/lib/gdb/dap/server.py b/gdb/python/lib/gdb/dap/server.py index 0f3991d..8f2a531 100644 --- a/gdb/python/lib/gdb/dap/server.py +++ b/gdb/python/lib/gdb/dap/server.py @@ -59,7 +59,9 @@ class NotStoppedException(Exception): class CancellationHandler: def __init__(self): # Methods on this class acquire this lock before proceeding. - self.lock = threading.Lock() + # A recursive lock is used to simplify the 'check_cancel' + # callers. + self.lock = threading.RLock() # The request currently being handled, or None. self.in_flight_dap_thread = None self.in_flight_gdb_thread = None @@ -72,11 +74,23 @@ class CancellationHandler: try: with self.lock: self.in_flight_dap_thread = req + # Note we do not call check_cancel here. This is a bit of + # a hack, but it's because the direct callers of this + # aren't prepared for a KeyboardInterrupt. yield finally: with self.lock: self.in_flight_dap_thread = None + def check_cancel(self, req): + """Check whether request REQ is cancelled. + If so, raise KeyboardInterrupt.""" + with self.lock: + # If the request is cancelled, don't execute the region. + while len(self.reqs) > 0 and self.reqs[0] <= req: + if heapq.heappop(self.reqs) == req: + raise KeyboardInterrupt() + def cancel(self, req): """Call to cancel a request. @@ -88,7 +102,7 @@ class CancellationHandler: gdb.interrupt() else: # We don't actually ignore the request here, but in - # the 'starting' method. This way we don't have to + # the 'check_cancel' method. This way we don't have to # track as much state. Also, this implementation has # the weird property that a request can be cancelled # before it is even sent. It didn't seem worthwhile @@ -105,10 +119,7 @@ class CancellationHandler: return try: with self.lock: - # If the request is cancelled, don't execute the region. - while len(self.reqs) > 0 and self.reqs[0] <= req: - if heapq.heappop(self.reqs) == req: - raise KeyboardInterrupt() + self.check_cancel(req) # Request is being handled by the gdb thread. self.in_flight_gdb_thread = req # Execute region. This may be interrupted by gdb.interrupt. @@ -152,6 +163,7 @@ class Server: "command": params["command"], } try: + self.canceller.check_cancel(req) if "arguments" in params: args = params["arguments"] else: -- cgit v1.1