Skip to content

fix issue 52 - #67

Merged
inducer merged 1 commit into
inducer:masterfrom
bukzor:two-set-trace-issue52
Mar 19, 2013
Merged

fix issue 52#67
inducer merged 1 commit into
inducer:masterfrom
bukzor:two-set-trace-issue52

Conversation

@bukzor

@bukzor bukzor commented Mar 18, 2013

Copy link
Copy Markdown
Contributor

I believe this is the right fix, and although this fix belongs in bdb.py, lets test it out in pudb first.

@bukzor

bukzor commented Mar 18, 2013

Copy link
Copy Markdown
Contributor Author

I had expected this pull request to show up on #52.
Owell.

@asmeurer

Copy link
Copy Markdown
Collaborator

GitHub only recognizes # + issue number, not issue + issue number.

@asmeurer

Copy link
Copy Markdown
Collaborator

At the very least open a Python core issue about this. I'm interested to see how the core devs feel about this patch. Definitely if they suggest a better solution for stdlib bdb, then we should also apply that here instead of this.

@asmeurer

Copy link
Copy Markdown
Collaborator

With that being said, this does appear to completely solve #52 as far as I was able to reproduce it, both for pressing n onto set_trace and for pressing Ctrl-C on an atomic operation like time.sleep(10).

inducer added a commit that referenced this pull request Mar 19, 2013
@inducer
inducer merged commit c4a122f into inducer:master Mar 19, 2013
@inducer

inducer commented Mar 19, 2013

Copy link
Copy Markdown
Owner

Very cool. Thank you for your work on this.

I'm glad this removes the poor behavior around multiple set_traces, I can't claim I understand what's going on. What misbehavior is this fixing? Note that pdb works fine even in the face of multiple set_traces. If this were upstreamed, would pdb still work?

@bukzor

bukzor commented Mar 19, 2013

Copy link
Copy Markdown
Contributor Author

Very cool. Thank you for your work on this.

You're quiet welcome. I'm glad you like it.

Note that pdb works fine even in the face of multiple set_traces.

That's not entirely true. You can reproduce the bad old pudb behavior like so:

from pdb import Pdb
debugger = Pdb()
debugger.set_trace()
debugger.set_trace()

What misbehavior is this fixing?

There's two parts to this. Firstly, pudb caches and re-uses its debugger objects. Apparently this is not a use case that occurs elsewhere, although I see no reason at all that it should malfunction. Secondly, bdb clears its debugger state before it's done executing code in bdb.py. Because this state includes self.stopframe, it mistakenly stops in bdb code when it should not. The "stop frame" is necessary to the correct behavior of the next debugging command; the debugger should not stop on any frame below it.

Equivalently, this patch also fixed the issue, but I had Benjamin Peterson look at it (briefly) and we couldn't find any reason to reset at this point in the code, and the unit tests (such as they are) pass without resetting here.

diff -r ef8ea052bcc4 Lib/bdb.py
--- a/Lib/bdb.py        Sun Mar 17 22:06:18 2013 -0400
+++ b/Lib/bdb.py        Mon Mar 18 22:42:42 2013 -0700
@@ -216,7 +216,9 @@
         """
         if frame is None:
             frame = sys._getframe().f_back
+        old_stopframe = getattr(self, 'stopframe', None)
         self.reset()
+        self.stopframe = old_stopframe
         while frame:
             frame.f_trace = self.trace_dispatch
             self.botframe = frame

If this were upstreamed, would pdb still work?

Yes. It would work exactly as well as pudb now works. Because bdb/pdb have poor test coverage, I want to flesh out exactly how well pudb now works :) Once we know that, the upstream argument becomes easy. I already know how I'd unit test this I think.

Hope that clears this up a bit.

@bukzor
bukzor deleted the two-set-trace-issue52 branch March 19, 2013 05:44
hwalinga added a commit to hwalinga/pudb that referenced this pull request Nov 29, 2024
…during postmortem.

This fixes inducer#607. Making the Debugger a singleton should be enough to fix inducer#607.
HOWEVER, pytest def postmortem() calls .reset() which reset the stopframe.
Somehow, this causes the debugger to stop somewhere inside internal debugger source code,
instead of in the code the postmortem should be done.

So, this also includes an overrid eof the .reset() method and detects when the debugger
is already running and does not reset the stopframe. See also inducer#67.
hwalinga added a commit to hwalinga/pudb that referenced this pull request Nov 29, 2024
…during postmortem.

This fixes inducer#607. Making the Debugger a singleton should be enough to fix inducer#607.
HOWEVER, pytest def postmortem() calls .reset() which resets the stopframe.
Somehow, this causes the debugger to stop somewhere inside internal debugger source code,
instead of in the code the postmortem should be done.

So, this also includes an override of the .reset() method and detects when the debugger
is already running and does not reset the stopframe. See also inducer#67.
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.

3 participants