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

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

On 2/23/24 18:27, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> The idea is that this interrupt triggers while in fn here in message (a nested
> Tom> function of send_gdb_with_response):
> Tom> ...
> Tom>     def message():
> Tom>         try:
> Tom>             val = fn()
> Tom>             result_q.put(val)
> Tom>         except (Exception, KeyboardInterrupt) as e:
> Tom>             result_q.put(e)
> Tom> ...
> Tom> but instead it triggers outside the try/except.
> 
> Thanks for the clear explanation.
> 
> Tom> I left the two "sleep 0.2" in the test-case, I didn't see a reason to remove
> Tom> those.
> 
> Those are there to try to make the test actually interrupt the command
> in question -- removing them would make it more likely to just interrupt
> the surrounding dispatch machinery, which isn't interesting.
> 

Posted v2 here ( 
https://sourceware.org/pipermail/gdb-patches/2024-February/206821.html ).

Ack, dropped that bit in the commit message in v2.

> Tom> @@ -435,12 +466,19 @@ def send_gdb_with_response(fn):
>   
> It seems to me that send_gdb also has this issue -- that is, the
> cancellation could arrive just in time to cause a KeyboardInterrupt.
> 

Fixed in v2 (though I'm not sure if there's a test-case exercising this).

> Also I wonder how multi-step / DAP-thread commands like 'continue' are
> handled.
> 

I think that should be handled in v2.

> Tom>      def message():
> Tom>          try:
> Tom> -            val = fn()
> Tom> +            if not _server.canceller.enter_interruptable():
> Tom> +                raise KeyboardInterrupt()
> Tom> +            try:
> Tom> +                val = fn()
> Tom> +            finally:
> Tom> +                _server.canceller.exit_interruptable()
> 
> This kind of thing is better with a context manager.
> 

Indeed, used that in v2.

> Tom>      send_gdb(message)
> Tom> +    if _server.canceller.is_cancel_pending():
> Tom> +        result_q.put(KeyboardInterrupt())
> Tom>      val = result_q.get()
> 
> I don't understand how this addition has an effect.  It seems to me that
> since a Queue is FIFO, and since the response Queue is only used for a
> single value, that this addition will just be ignored.
> 

Agreed, it's unnecessary.  Dropped in v2.

> FWIW, cancellation is inherently racy.  I'm not opposed to trying to
> make this more reliable but I think it may be pretty hard.
> 
> Maybe some kind of installable exception handler that is run when
> post_event fails would be one way.  Not sure.

Not sure either.  Anyway I hope v2 is an improvement.

Thanks,
- Tom