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.

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

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

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

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a3993e
Make the Debugger a singleton and do not reset the stopframe to None …
hwalinga Nov 29, 2024
9f1a22a
Have more precise control on when to no reset the stopframe.
hwalinga Nov 29, 2024
a161970
Adding more comments as to what the problem is that pytest is doing.
hwalinga Dec 20, 2024
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
43 changes: 39 additions & 4 deletions pudb/debugger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,15 +183,24 @@

# {{{ 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:
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 +223,32 @@ def __init__(self, stdin=None, stdout=None, term_size=None, steal_output=False,
# 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?

calling .reset() in src/_pytest/debugging.py::post_mortem
hwalinga marked this conversation as resolved.
Show resolved Hide resolved
causes the self.stopframe to be set to None.
This pauses the debugger somewhere in the source code of the debugger. See #67

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

Related #607, #52
"""
import linecache
linecache.checkcache()
self.botframe = None
if not self._pytest_postmortem:
self.stopframe = None
else:
self._pytest_postmortem = False
self.returnframe = None
self.quitting = False
self.stoplineno = 0
Copy link
Owner

Choose a reason for hiding this comment

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

Would it be possible to call into Bdb.reset for some of this? We're already copy-pasting too much of Bdb's guts into pudb... (If not, add a comment why not.)

Copy link
Author

Choose a reason for hiding this comment

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

When calling Bdb.reset the debugger stops around there. self.stopframe = None is the offending line. Calling Bdb.reset is a no-go. It is the exact same problem for #67 where also code is copies from Bdb in here. The problem now is that pytest is calling .reset, so that is why the override is needed and to check for the pytest postmortem state. I can add a link to the source of pytest as well, but I believe the docstring of this method explain already enough.

Do you think there needs to be more information in the docstring of this method to explain why it needs to be overriden?


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