gh-132975: Improve Remote PDB interrupt handling #133223
Conversation
Because our goal is to proxy interrupt signals to the remote process,
it's important that we only accept those signals at moments when we're
ready to handle them and send them on. In particular, we're prepared to
handle them when reading user input for a prompt, and when waiting for
the server to send a new prompt after our last command. We do not want
to accept them at other times, like while we're in the middle of
printing output to the screen, as otherwise a
while True: "Hello"
executed at the PDB REPL can't reliably be Ctrl-C'd: it's more likely
that the signal will arrive while the client is printing than while it's
waiting for the next line from the server, and if so nothing will be
prepared to handle the `KeyboardInterrupt` and the client will exit.
Previously we worked with a file object created with `socket.makefile`, but on Windows the `readline()` method of a socket file can't be interrupted with Ctrl-C, and neither can `recv()` on a socket. This refactor lays the ground work for a solution this this problem.
Since a socket read can't be directly interrupted on Windows, use the `selectors` module to watch for either new data to be read from that socket, or for a signal to arrive (leveraging `signal.set_wakeup_fd`). This allows us to watch for both types of events and handle whichever arrives first.
This unfortunately cannot be applied to Windows, which has no way to trigger a SIGINT signal in a remote process without running code inside of that process itself.
godlygeek
force-pushed
the
improve_remote_pdb_interrupt_handling
branch
from
098b752 to
ed664b8
Compare
|
OK, this is ready for a review (and needs the skip-news label) |
There was a problem hiding this comment.
Okay so _readline is to make it possible for the client to be interrupted while receiving data from the server. You have a _handle_sigint outside of each _readline call which enables SIGINT. What if the interrupt happens before your try ... finally ... block in _readline? old_wakeup_fd would not be restored I think? Not sure if other evil thing could happen.
As far as I can tell, if you want to set your signal handler in _readline() anyway, you should just restore the signal handling in readline() - you wouldn't need to wrap every call with _handle_sigint and less racing concerns.
I see that you put some effort to achieve a "delayed" interrupt compared "ignored" ones on Windows. Is that critical? We will have much less code if we treat them the same. I think interrupting at ignored region should be the rare case anyway right?
I'm not an expert in signal, if we have multiple SIGINT during the blackout region, will we trigger multiple handlers?
signal.default_int_handler is not documented, but it is used a few times in stdlib. If that's the only handler we are going to use for _handle_sigint, should we just put it into the function and call it _unblock_sigint so it's obvious that it has the opposite effect of _block_sigint?
Also, we need a news entry for this PR. This is a user observable change (even though it in alpha phase).
Ooh, yes, that's a bug. I'll take another look at that.
It depends... it can be very common, depending on what's happening. If you do the
I think I've thought of an idea for how to treat them the same, and get the "delayed" interrupts even on Windows. I'll get back to you on this...
I thought news entries were only for things that have made it into releases, not for bug fixes to stuff that's never been released. But OK, I can add one if it's needed. |
Okay so what if the server is somehow dumping a lot of data to the client? Your ideal solution is to be able to delay the interrupt signal then send it to the server right? |
Yes, exactly. If the user hits Ctrl-C while we're printing data, we want to set that Ctrl-C aside for later, and handle it the next time we're prepared for it - either when we next call |
|
So we've got 3 different ways that we want to handle SIGINT, at different points in the client:
I think I can make this simpler and more robust by using one SIGINT handler that does all 3 of those things, based on some flags being set by the application to tell it which mode it should be in. I'm gonna try that out, and if it does work it ought to be much safer than setting different handlers all over the place. |
It seems like this is raised explicitly in some cases.
|
OK, I switched this around to use a single signal handler and some flags to tell it what to do. It's about the same amount of code, but I do think it's easier to reason about now. Please take another look. |
No tests yet, mostly because I'm still trying to figure out if there's something better we can do for Windows or not. This PR solves #132975 pretty thoroughly for Unix, but Windows still has the problem that a |
|
OK - I've landed on something that works for Windows. I've gone with the minimal thread approach that we discussed in #132975 (comment) but only for Windows. We could use this approach for Unix too. The advantage of using it only for Windows is that we don't need to do the more complex thing with more failure modes on all the other platforms, but the disadvantage of using it only for Windows is that we now have two different code paths to maintain and test. Take a look and let me know a) if you're OK with using this approach for Windows, and b) if you'd like to also use this approach for Unix for the sake of consistency or if you'd rather stick with the simpler |
|
Did we create a thread that will be in the process forever every time we attach to the process? |
|
No, the thread dies 0.25 seconds after |
|
Looks like I did break the graceful error handling for another PDB session already being attached (the server sends an error and disconnects, the client is still waiting for the server to make the second connection for interrupt signals). I'll fix that once you confirm you're OK with the overall approach. |
There was a problem hiding this comment.
I'm okay with the overall approach. A few minor points:
- Could you test that the current code works with commands? I was a bit worried about how you use commands there - will it work when you expand those and write as a file?
- We have a few random functions that are only about remote pdb lying around now. It may not be a huge issue, but I was wondering if it makes more sense to put them into
_PdbServerandPdbClientasclassmethods - so it's clear that they belong to remote pdb and we know which end we should call it.
I'm just gonna change that. I was trying to be clever and avoid adding an extra parameter to
Rather than trying to smuggle client setup logic in the commands, I'll just be explicit about it, and do it in
Sure, should be easy enough. |
Do you want to use the thread approach on both Unix and Windows (less code, greater consistency, less to test, more potential failure modes on Unix, works in truly remote scenarios) or do you want to have the client use |
|
I'm leaning towards using the same approach on all platforms. It's overkill for what we actually need on Unix, but it'll be less code and fewer tests, and fewer platform-specific differences that maintainers need to remember. If we ever find a reason why this causes problems on Unix, we can always revert back to using |
Let the caller of `_connect` tell us whether to start the thread.
This gives us greater flexibility to reorder things or add things in the future without breaking backwards compatibility.
|
Yeah I think consistent code on different platforms is probably easier to maintain. |
Resolved that, moved the |
Ugh, no, there's a problem with that: it won't interrupt IO on Unix. I do think we need two different paths. Or alternatively, the signal listening thread can spawn a subprocess that sends a SIGINT to its parent process (more complex, more failure modes, but works in a truly remote scenario with the client and server on different hosts). |
|
I'm back to thinking this is ready to merge. I've switched it back to sending the signal from the client on Unix, so that it interrupts IO on the main thread. |
|
In plan to land this today so it makes it to beta 1 so people can try it unless @gaogaotiantian thinks there are still any big concerns pending that cannot be addressed later. |
|
@godlygeek could you resolve the merge conflicts? If you don't have time I can try to do it in a couple of hours |
Fix several problems in how Ctrl-C works in the Remote PDB client:
for x in itertools.count(): x). Fix this by explicitly blocking SIGINT signals except for the times when we're prepared to handle them and ready to send them on to the remote.sockfile.readline()call made while waiting for the next message from the remote. Work around this by usingselectorsto listen for updates on either of two streams - either a message from the remote, or a signal arriving and being written to asocketpairby our signal handler.Making this a draft PR for now to check whether the selectors and socketpair usage need us to skip tests on additional platforms.