8000 gh-131591: Allow pdb to attach to a running process by godlygeek · Pull Request #132451 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-131591: Allow pdb to attach to a running process 8000 #132451

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.

Sign up for GitHub

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

Merged
merged 40 commits into from
Apr 25, 2025
Merged

gh-131591: Allow pdb to attach to a running process #132451

Changes from 1 commit
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
5225333
Allow pdb to attach to a running process
godlygeek Apr 12, 2025
78a3085
Remove 2 unused _RemotePdb instance attributes
godlygeek Apr 15, 2025
90e0a81
Reduce duplication for 'debug' command
godlygeek Apr 15, 2025
e44a670
End commands entry on 'end' and ^C and ^D
godlygeek Apr 15, 2025
e837246
Set the frame for remote pdb to stop in explicitly
godlygeek Apr 15, 2025
27efa97
Fix an unbound local in an error message
godlygeek Apr 15, 2025
557a725
Clean up remote PDB detaching
godlygeek Apr 15, 2025
7f7584a
Allow ctrl-c to interrupt a running process
godlygeek Apr 17, 2025
5666ffb
Automatically detach if the client dies unexpectedly
godlygeek Apr 17, 2025
325f166
Clear _last_pdb_instance on detach
godlygeek Apr 17, 2025
72830e2
Refuse to attach if another PDB instance is installed
godlygeek Apr 17, 2025
27c6780
Handle the confirmation prompt issued by 'clear'
godlygeek Apr 18, 2025
baaf28a
Make message and error handle non-string args
godlygeek Apr 18, 2025
e61cc31
Add some basic tests
pablogsal Apr 18, 2025
5d59ce1
Don't use deprecated method
pablogsal Apr 18, 2025
09adb2b
Try to prevent a PermissionError on Windows
godlygeek Apr 18, 2025
600aa05
Address review comments
godlygeek Apr 20, 2025
0601f10
Add protocol versioning and support -c commands
godlygeek Apr 20, 2025
5a1755b
Fix tests to match new _connect signature for protocol versioning/com…
godlygeek Apr 21, 2025
f184e4e
Add some comments describing our protocol
godlygeek Apr 21, 2025
82b71f8
Use the 'commands' state for '(com)' prompts
godlygeek Apr 22, 2025
3986c17
Remove choices parameter from _prompt_for_confirmation
godlygeek Apr 22, 2025
2e69667
Rename _RemotePdb to _PdbServer
godlygeek Apr 22, 2025
55adbcc
Avoid fallthrough in signal handling
godlygeek Apr 22, 2025
0bda5c2
Fix handling of a SystemExit raised in normal pdb mode
godlygeek Apr 22, 2025
5e93247
Address nit
godlygeek Apr 22, 2025
f799e83
Use textwrap.dedent for test readability
godlygeek Apr 22, 2025
46fb219
Drop dataclasses dependency
godlygeek Apr 22, 2025
1ec9475
Combine the two blocks for handling -p PID into one
godlygeek Apr 22, 2025
662c7eb
Add a news entry
godlygeek Apr 22, 2025
ac36d7d
Skip remote PDB integration test on WASI
godlygeek Apr 22, 2025
f06d9c2
Two small things missed in the previous fixes
godlygeek Apr 22, 2025
715af27
Remove call to set_step in interrupt handler
godlygeek Apr 22, 2025
c654fdf
More tests
pablogsal Apr 23, 2025
205bc55
More tests
pablogsal Apr 23, 2025
bbe784b
More tests
pablogsal Apr 23, 2025
30cb537
Add what's new entry
pablogsal Apr 23, 2025
659556f
use dedent
pablogsal Apr 23, 2025
6c2d970
Add synchronization to test_keyboard_interrupt
godlygeek Apr 24, 2025
100be44
Stop sending a "signal" message in test_keyboard_interrupt"
godlygeek Apr 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Try to prevent a PermissionError on Windows 
If `NamedTemporaryFile.__exit__` tries to delete the connect script
before the remote process has closed it, a `PermissionError` is raised
on Windows. Unfortunately there is no synchronization that we can use to
ensure that the file is closed before we try to delete it. This makes
the race less likely to occur by making the `with` block contain more
code, so that more time passes before we attempt to delete the file.

Co-authored-by: Chris Eibl <138194463+chris-eibl@users.noreply.github.com>
  • Loading branch information
@godlygeek @chris-eibl
godlygeek and chris-eibl authored Apr 18, 2025
commit 09adb2b9b0aa54db6ff8a3c3dbb98c53a09c7d9d
32 changes: 16 additions & 16 deletions Lib/pdb.py
View file
Open in desktop
8000
Original file line number Diff line number Diff line change
Expand Up @@ -3044,31 +3044,31 @@ def attach(pid):
with closing(socket.create_server(("localhost", 0))) as server:
port = server.getsockname()[1]

with tempfile.NamedTemporaryFile("w", delete_on_close=False) as script:
script.write(
with tempfile.NamedTemporaryFile("w", delete_on_close=False) as connect_script:
connect_script.write(
f'import pdb, sys\n'
f'pdb._connect("localhost", {port}, sys._getframe().f_back)\n'
)
script.close()
sys.remote_exec(pid, script.name)
connect_script.close()
sys.remote_exec(pid, connect_script.name)

# TODO Add a timeout? Or don't bother since the user can ^C?
client_sock, _ = server.accept()

with closing(client_sock):
sockfile = client_sock.makefile("rwb")
with closing(client_sock):
sockfile = client_sock.makefile("rwb")

with closing(sockfile):
with tempfile.NamedTemporaryFile("w", delete_on_close=False) as script:
script.write(
'import pdb, sys\n'
'if inst := pdb.Pdb._last_pdb_instance:\n'
' inst.set_step()\n'
' inst.set_trace(sys._getframe(1))\n'
)
script.close()
with closing(sockfile):
with tempfile.NamedTemporaryFile("w", delete_on_close=False) as interrupt_script:
interrupt_script.write(
Copy link
Member

Choose a reason for hiding this comment

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

Okay I think I found a problem here. If you do

while True:
    pass

from client, the server will stuck in the loop forever.

Because set_trace() will not break in pdb - it's already tracing.

I thought you could simply do a signal.raise_signal(signal.SIGINT) here, but it seems like the exceptions inside sys.exec_remote is not propagated?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exceptions aren't propagated out of the script run by sys.remote_exec(), so while signal.raise_signal() will work, if that signal handler sets an exception it'll just wind up as an unraisable exception and discarded.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. This is sort of a PDB problem in general, isn't it?

Signal handlers only run on the main thread, so if you're using PDB in a multi-threaded program and you do

while True:
    pass

on any thread but the main thread, that thread will be stuck forever, right?

The only thing that remote PDB is making worse is that getting stuck will happen for all N threads, instead of for N-1 of them 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exceptions aren't propagated out of the script run by sys.remote_exec()

It might make sense to special case KeyboardInterrupt in particular and allow it to pass through. We have the eval 8000 loop swallow exceptions raised by the injected script because the main application isn't expecting them or prepared to handle them, but KeyboardInterrupt is special because it's always allowed to be raised from just about anywhere (at least if the app hasn't changed the default SIGINT handler), and it can't be raised accidentally by the injected script and would only be raised deliberately (or if the user ctrl-c'd the injected script - but even in that case, propagating it is probably the best choice...)

@pablogsal mentioned to me earlier today that he has some ideas about better ways to do the interrupts, but we haven't had a chance to discuss it yet.

Copy link
Member

Choose a reason for hiding this comment

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

It's true for multi-thread programs. However, pdb is always horrible at debugging multi-thread programs. There are millions of issues there so that's not our major concern now. I really hope that a single-thread program can work with remote pdb.

Copy link
Contributor Author
@godlygeek godlygeek Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Could this be fixed by running evaluated code with tracing enabled? Or would that break more than it fixes?

Copy link
Member

Choose a reason for hiding this comment

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

The issue is that the evaluated code is in a trace callback, so it won't call any other callback functions - otherwise it's infinite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, but we could use sys.call_tracing() when calling that evaluated code - probably disabling the breakpoints first and reenabling them once it has finished executing. That should fix this issue, but I'm not sure what other issues it might lead to...

Copy link
Member

Choose a reason for hiding this comment

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

I think that will disable the breakpoints in the evaluated code?

Copy link
Contributor Author
@godlygeek godlygeek Apr 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

8000

Aren't they already disabled, since we're running the evaluated code from a trace function and not calling it with sys.call_tracing()?

If I do:

(Pdb) import functools
(Pdb) b functools.cache
Breakpoint 1 at /opt/bb/lib/python3.13/functools.py:679
(Pdb) functools.cache(print)
<functools._lru_cache_wrapper object at 0x7f2309b5f1c0>

the breakpoint doesn't get hit.

'import pdb, sys\n'
'if inst := pdb.Pdb._last_pdb_instance:\n'
' inst.set_step()\n'
Copy link
Member

Choose a reason for hiding this comment

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

Why set_step()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I based it on the sigint handler that regular PDB installs:

    def sigint_handler(self, signum, frame):
        if self.allow_kbdint:
            raise KeyboardInterrupt
        self.message("\nProgram interrupted. (Use 'cont' to resume).")
        self.set_step()
        self.set_trace(frame)

If that's not the right thing to do, happy to change it.

Copy link
Member

Choose a reason for hiding this comment

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

Okay that piece of code is from 15 years ago. I don't believe set_step() is needed here. I think you can remove it - we won't risk breaking existing code. I'll probably do a better analysis and test in the future and remove it from sigint_handler if it's not doing anything.

' inst.set_trace(sys._getframe(1))\n'
)
interrupt_script.close()

_PdbClient(pid, sockfile, script.name).cmdloop()
_PdbClient(pid, sockfile, interrupt_script.name).cmdloop()


# Post-Mortem interface
Expand Down
Loading
0