Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-118714: Make the pdb post-mortem restart/quit behavior more reasonable #118725

Merged
merged 4 commits into from
Jul 3, 2024

Conversation

gaogaotiantian
Copy link
Member

@gaogaotiantian gaogaotiantian commented May 7, 2024

Currently the restart and quit behavior of pdb post-mortem debugging mode is a bit weird.

As the issue mentioned, if the user uses quit or hit Ctrl+D in post-mortem debugging mode, the debugger will quit, but with a message of the script will be restarted. If the user uses restart command, an exception will be raised and the debugger will quit due to the exception.

This PR is to clean up the bahaviors. In post-mortem debugging, the user can:

  • use restart to restart
  • use cont / step to restart as instructed
  • use quit or Ctrl+D to quit

The message will always informing the users that they are restarting the program if they try to restart, and will quietly quit when they quit.

I would consider this a bug fix so it should not be affected by beta freeze to be merged into 3.13.

@gaogaotiantian gaogaotiantian added 3.12 bugs and security fixes 3.13 bugs and security fixes labels May 12, 2024
@gaogaotiantian
Copy link
Member Author

Hi @iritkatriel , as this is a bug fix, I'd like to make it simple and correct. As I said, printing stuff in do_restart will raise the issue of incorrectly using restart command when the user should not (pdb brought up by breakpoint() and there's no way to restart). I plan to clean up the structure to disallow users to use certain commands (run/restart) when they are not supposed to and generate some warnings in some cases (Ctrl+D in inline pdb will just stop the process). All above require some feature work (customize pdb.Pdb with some arguments like allow_restart, which is only set when pdb is brought up by command line interface) and should not be backported.

So could we maybe do the quick fix first and backport, then I'll make it better in 3.14.

@@ -0,0 +1 @@
Make the :mod:`pdb` post-mortem restart/quit message match the behavior
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't very clear. Also, it seems inconsistent with the title of this PR. Are you changing the behaviour (as the title says) or changing the message to match the behaviour?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both behavior and message were changed. There were two cases involved, both under post-mortem debugging:

  1. If the user does restart, it actually restarts the program, instead of raising an error and exit. (This is the behavior change).
  2. If the user does quit, it quits the program without printing the line saying "we'll restart the program".

I think I should make the behavior change clearer (it's considered a bug fix because quitting the debugger when doing restart feels wrong).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to change this after the review. Just changed it. Let me know if you think this could be better.

@iritkatriel
Copy link
Member

If you want to backport you need the backport labels. We use the green ones on issues.

@gaogaotiantian gaogaotiantian added needs backport to 3.13 bugs and security fixes and removed 3.12 bugs and security fixes 3.13 bugs and security fixes labels Jul 3, 2024
@gaogaotiantian
Copy link
Member Author

Yeah I don't know why I added the issue tags back then. I'll only backport this to 3.13, as that's the version with the behavior change. I'll leave 3.12 alone.

@gaogaotiantian gaogaotiantian merged commit e245ed7 into python:main Jul 3, 2024
38 checks passed
@miss-islington-app
Copy link

Thanks @gaogaotiantian for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

@gaogaotiantian gaogaotiantian deleted the cleanup-pdb-exit branch July 3, 2024 18:30
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 3, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jul 3, 2024

GH-121346 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 3, 2024
gaogaotiantian added a commit that referenced this pull request Jul 3, 2024
… reasonable (GH-118725) (#121346)

gh-118714: Make the pdb post-mortem restart/quit behavior more reasonable (GH-118725)
(cherry picked from commit e245ed7)

Co-authored-by: Tian Gao <[email protected]>
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants