GeezerVison

OK, this is ready for a review (and needs the skip-news label)

CC @gaogaotiantian @pablogsal

What if the interrupt happens before your try ... finally ... block in _readline? old_wakeup_fd would not be restored I think?

Ooh, yes, that's a bug. I'll take another look at that.

I think interrupting at ignored region should be the rare case anyway right?

It depends... it can be very common, depending on what's happening. If you do the while True: "x" loop I mentioned above, most of the time is spent printing, and comparatively little time is spent reading input.

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 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...

Also, we need a news entry for this PR. This is a user observable change (even though it in alpha phase).

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.

It depends... it can be very common, depending on what's happening. If you do the while True: "x" loop I mentioned above, most of the time is spent printing, and comparatively little time is spent reading input.

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?

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 input() to show the user a prompt, or when we next read a message from the socket, whichever comes first. This is far easier than trying to handle the interrupt asynchronously at arbitrary points.

So we've got 3 different ways that we want to handle SIGINT, at different points in the client:

• When we've called input(), we want a KeyboardInterrupt to be raised by the signal handler, breaking us out of the input() call.
• When we've called socket.recv(), we want a flag to be set and something to be written to the wakeup fd, so that we can break out of the select() call and raise a KeyboardInterrupt ourselves.
• In all other cases we want a flag to be set that we can check later, when we're about to request input() from the user or a recv() from the remote.

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.

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.

Also do we still have any coverage for the new feature?

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 while True: pass executed at the PDB prompt can't be interrupted, and I haven't yet figured out a solution I'm happy with.

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 os.kill() version on Unix.

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 atexit handlers run or when the client disconnects, whichever comes first.

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.

1. Could you test that the current code works with commands? I was a bit worried about how you use commands there

I'm just gonna change that. I was trying to be clever and avoid adding an extra parameter to _connect, but that's ultimately the cause of this problem:

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).

Rather than trying to smuggle client setup logic in the commands, I'll just be explicit about it, and do it in _connect before we start processing the commands, and before we check if there's another session attached.

2. 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 _PdbServer and PdbClient as classmethods

Sure, should be easy enough.

I'm okay with the overall approach.

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 os.kill() on Unix (more code, less consistency, more to test, fewer potential failure modes on Unix, not usable in truly remote scenarios)?

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 os.kill() directly as a bug fix.

Yeah I think consistent code on different platforms is probably easier to maintain.

Overall I think it's good for now. Need to resovle the conflict though.

Resolved that, moved the contextlib imports onto one line, and added some tests now that we've settled on the approach. I think this is ready to merge if you're happy with it.

I'm leaning towards using the same approach on all platforms.

Yeah I think consistent code on different platforms is probably easier to maintain.

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