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

Make the Debugger a singleton and do not reset the stopframe to None during postmortem. #667

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
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
54 changes: 50 additions & 4 deletions pudb/debugger.py
Original file line number Diff line number Diff line change
Expand Up @@ -183,15 +183,29 @@

# {{{ debugger interface

class Debugger(bdb.Bdb):
class Singleton(type):
_instance = None

def __call__(cls, *args, **kwargs):
if not cls._instance:
cls._instance = super(Singleton, cls).__call__(*args, **kwargs)
else:
# Instantiating the debugger again is a pytest problem
# when starting a postmortem.
# Otherwise calling _get_debugger should be called.
# This flag keeps track of the pytest postmortem situation to disable
# a problematic line in Bbd.reset()
cls._pytest_postmortem = True
Copy link
Owner

Choose a reason for hiding this comment

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

How can we be sure that this case is only hit by pytest, as the code suggests? Could the condition under which this triggers be made a bit more narrow?

return cls._instance


class Debugger(bdb.Bdb, metaclass=Singleton):
_current_debugger = []
_pytest_postmortem = False

def __init__(self, stdin=None, stdout=None, term_size=None, steal_output=False,
_continue_at_start=False, tty_file=None, **kwargs):

if Debugger._current_debugger:
raise ValueError("a Debugger instance already exists")

# Pass remaining kwargs to python debugger framework
bdb.Bdb.__init__(self, **kwargs)
self.ui = DebuggerUI(self, stdin=stdin, stdout=stdout, term_size=term_size)
Expand All @@ -214,6 +228,38 @@
# Okay, now we have a debugger
self._current_debugger.append(self)
Comment on lines 228 to 229
Copy link
Owner

Choose a reason for hiding this comment

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

Could we get rid of _current_debugger in favor of the singleton mechanism?

Copy link
Author

Choose a reason for hiding this comment

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

That is logic in place for _get_debugger. This has to remain. That is all fine.

Trying to instantiate a debugger again (and thus previously hitting the ValueError) is what pytest is doing. Thus my code only works around that specific pytest problem. All logic around _get_debugger is fine, but pytest is just not calling it.

Copy link
Owner

Choose a reason for hiding this comment

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

This has to remain.

Why? There are now essentially two copies of singleton-management code floating around. My question is aimed at reducing that to one.


def reset(self):
"""Set values of attributes as ready to start debugging.

Override from Bdb.reset()

When pytest starts a postmortem analysis, but the debugger is already active,
Copy link
Owner

Choose a reason for hiding this comment

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

active

What does "active" mean in this context? How is this circumstance detected?

.reset() in src/_pytest/debugging.py::post_mortem is called.
https://github.com/pytest-dev/pytest/blob/868e1d225e443984a6aa29cfde2d1231eb03ed41/src/_pytest/debugging.py#L396

When Bdb.reset() is called this causes the self.stopframe to be set to None.
That immidiatly pauses the debugger somewhere in the source code of the debugger.

Check warning on line 241 in pudb/debugger.py

View workflow job for this annotation

GitHub Actions / Typos

"immidiatly" should be "immediately".
The exact same problem as for #67.

We detect using _pytest_postmortem that this is the case and do not set the
stopframe to None in that specific scenario.

Related #607, #52
"""
import linecache
linecache.checkcache()
self.botframe = None

if not self._pytest_postmortem:
# The problematic line of Bbd.reset:
self.stopframe = None
else:
self._pytest_postmortem = False

self.returnframe = None
self.quitting = False
self.stoplineno = 0

def __del__(self):
# according to https://stackoverflow.com/a/1481512/1054322, the garbage
# collector cannot be relied on to call this, so we call it explicitly
Expand Down
Loading