Re: [PATCH v2 2/2] [gdb/dap] Fix stray KeyboardInterrupt after cancel

[Tom de Vries <tdevries@suse.de>]

On 2/28/24 21:12, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Fix this by:
> Tom> - in CancellationHandler, renaming variable in_flight to in_flight_dap_thread,
> Tom>   and adding a variable in_flight_gdb_thread to be able to distinguish when
> Tom>   a request is in flight in the dap thread or the gdb thread.
> Tom> - adding a wrapper Cancellable to to deal with cancelling the wrapped
> Tom>   event
> Tom> - using Cancellable in send_gdb and send_gdb_with_response to wrap the posted
> Tom>   event
> Tom> - in CancellationHandler.cancel, only call gdb.interrupt if
> Tom>   req == self.in_flight_gdb_thread.
> 
> Thanks.  I like this patch a lot.
> 
> I have one question about it.
> 
> Tom> +    @contextmanager
> Tom> +    def interruptable_region(self, req):
> Tom> +        with self.lock:
> Tom> +            # If the request is cancelled, don't execute the region.
> Tom> +            while len(self.reqs) > 0 and self.reqs[0] <= req:
> Tom> +                if heapq.heappop(self.reqs) == req:
> Tom> +                    raise KeyboardInterrupt()
> Tom> +            # Request is being handled by the gdb thread.
> Tom> +            self.in_flight_gdb_thread = req
> 
> (see below)
> 
> Tom> +        try:
> Tom> +            # Execute region.  This may be interrupted by gdb.interrupt.
> Tom> +            yield None
> Tom> +        finally:
> Tom> +            # Request has been handled by the gdb thread,
> Tom> +            with self.lock:
> Tom> +                self.in_flight_gdb_thread = None
> Tom> +
> 
> I think it's possible for the interrupt to occur at the marked spot
> above -- in_flight_gdb_thread has been set and the lock has been
> released.
> 
> However, in this case it seems like in_flight_gdb_thread won't be reset?
> 
> I'm not really sure if this matters.  However, wouldn't moving the 'with'
> block inside the 'try' definitely fix this?
> 

Done in v3.

Thanks,
- Tom


> Tom