Opened 5 years ago

Last modified 4 years ago

#5795 enhancement new

Failure's startDebugMode should allow using a debugger other than pdb

Reported by: Julian Berman Owned by: Julian Berman
Priority: normal Milestone:
Component: core Keywords:
Cc: Jonathan Lange Branch: branches/failure-select-debugger-5795
branch-diff, diff-cov, branch-cov, buildbot
Author: julian, tomprince

Description (last modified by Thijs Triemstra)

Following #5794, Failure's startDebugMode function needs to be updated to take a debugger argument with a debugger it will use to do post-mortems.

Attachments (5)

startdebuggermode.patch (12.2 KB) - added by Julian Berman 4 years ago.
startdebuggermode.2.patch (14.0 KB) - added by Julian Berman 4 years ago.
startdebuggermode.3.patch (16.5 KB) - added by Julian Berman 4 years ago.
fixCapturingDebugger.patch (951 bytes) - added by Julian Berman 4 years ago.
startdebugmode.patch (7.9 KB) - added by Julian Berman 4 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 5 years ago by DefaultCC Plugin

Cc: Jonathan Lange added

comment:2 Changed 5 years ago by Thijs Triemstra

Component: trialcore
Description: modified (diff)

comment:3 Changed 5 years ago by Glyph

#6276 was a duplicate of this.

comment:4 Changed 5 years ago by Jean-Paul Calderone

What if the debugger the user selected doesn't have post-mortem functionality?

comment:5 in reply to:  4 Changed 5 years ago by Glyph

Replying to exarkun:

What if the debugger the user selected doesn't have post-mortem functionality?

In that case falling back to pdb would make sense; but pudb and pdbpp both have post-mortem functionality and those are the ones that I think people are generally most interested in. I suspect that if a new one arose, API-compatibility with pdb would be a goal for them too.

comment:6 Changed 4 years ago by Julian Berman

Keywords: review added

OK. Here's a patch.

This patch also introduces failure.stopDebugMode, which I've more than once needed to do cleanup in tests, including here.

Also, I should mention for reviewers that the debugger that most of us probably use who care about this (pudb) seems to have a bug? whereby it doesn't satisfy pdb's interface (it takes a tuple of exc_info rather than a traceback, which I've filed as https://github.com/inducer/pudb/issues/70 until I get a chance to look closer, so until that's fixed pudb won't actually work unless you make that change locally). pdbpp of course works, but that didn't even require this patch obviously.

Changed 4 years ago by Julian Berman

Attachment: startdebuggermode.patch added

comment:7 Changed 4 years ago by Julian Berman

Whoops. Failing test. Fixed.

Changed 4 years ago by Julian Berman

Attachment: startdebuggermode.2.patch added

comment:8 Changed 4 years ago by Tom Prince

Author: tomprince
Branch: branches/failure-select-debugger-5795

(In [38007]) Branching to failure-select-debugger-5795.

comment:9 Changed 4 years ago by Tom Prince

(In [38008]) Apply startdebuggermode.2.patch from Julian.

Refs: #5795

comment:10 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Julian Berman
  1. debugger needs a @param.
  2. I wonder if having _realInit and _debugInit on Failure would make sense, instead of what is done currently.
  3. I think the new default for debugger is incorrect.
  4. opt_debugger probably shouldn't have a docstring (see #6427).
  5. Might it make more sense for startDebugMode to take the post-mortem function, rather than the debugger module (since that is what it cares about?
    • That would mean the logic for determining whether the debugger supports post-mortems gets moved to trial.
    • If there is a debugger that doesnt support .post_mortem (or somebody wants to something different with the traceback), it would then be easy to add an option to call something arbitrary there.
  6. Is there a reason for the local import in test_setsUpFailureDebugMode?
  7. stopDebugMode is not tested.

comment:11 Changed 4 years ago by Julian Berman

Keywords: review added
Owner: Julian Berman deleted

OK. Phew.

  1. Done
  2. Like this, forgot why I didn't do it before, but did it now.
  3. Not sure what you mean, but as you'll see this patch takes a slightly different approach due to #5, so let me know if this still applies.
  4. Done.
  5. Yes, like this. Done.
  6. Yes, CapturingDebugger being mutated, but I fixed that with a not totally ideal but slightly more reasonable thing that still allows putting a debugger someplace where command line can find it.
  7. Done.

One thing I removed entirely here is _wrappedPdb, a function that was loading a local .pdbrc and readline. Both of those are present in the standard library's pdb now as of 2.6, so we don't need to do this anymore. (Besides the fact that it didn't actually work for this part of this patch, because pdb.post_mortem which was what failure.startDebugMode was directly calling, has no way to inject a custom pdb.Pdb instance).

Changed 4 years ago by Julian Berman

Attachment: startdebuggermode.3.patch added

comment:12 Changed 4 years ago by Julian Berman

Sorry, inadvertently renamed an attribute on CapturingDebugger which broke two runner tests, fixed those in an incremental patch. I may flesh out CapturingDebugger later since there are other debugger related tickets that I may eventually get to.

Changed 4 years ago by Julian Berman

Attachment: fixCapturingDebugger.patch added

comment:13 Changed 4 years ago by Tom Prince

Owner: set to Tom Prince

comment:14 Changed 4 years ago by Tom Prince

(In [38487]) Apply startdebuggermode.3.patch from Julian.

Refs: #5795

comment:15 Changed 4 years ago by Tom Prince

(In [38495]) Really apply startdebuggermode.3.patch and fixCapturingDebugger.patch from Julian.

Refs: #5795

comment:16 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Julian Berman
  1. It appears there are still a couple of test failures.
  2. Are there existing debuggers that raise BdbQuit? Looking at the code for bdb, it seems more like BdbQuit is used for internal communication, rather than something that is supposed to escape.
    • In fact, I'm not entirely sure why you are falling back to pdb here anyway?
  3. In the news file: failure.startDebugMode doesn't respect --debugger, trial calls startDebugMode in a way that respects --debugger.
  4. test_stopDebugMode is missing a docstring
  5. I wonder if replacing createAndCleanup with something like twisted.python.test.modules_helpers.replaceSysModules might be simpler?
    • This means you can use a shorter name for the debugger.
  6. test_noPostMortemMethod shouldn't use CapturingDebugger. (I dislike the mutation necessary to use it here.

comment:17 Changed 4 years ago by Julian Berman

Keywords: review added
Owner: Julian Berman deleted
  1. Fixed
  2. AFAIU they all do I think. pdb and pudb both do specifically, if you press q or otherwise quit. They're not wrapping anything special, they just pass through the quit process to bdb. I fixed / removed the fallback though, not sure what the thought process was there when I wrote it.
  3. Fixed
  4. Fixed
  5. Nice.
  6. Fixed.

Thanks for the review :)

Changed 4 years ago by Julian Berman

Attachment: startdebugmode.patch added

comment:18 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: set to Julian Berman
  1. Testing with the following patch applied prints out "Hi Here" which suggest that BdbQuit doesn't escape pdb.post_mortem.
    • twisted/python/failure.py

      diff --git twisted/python/failure.py twisted/python/failure.py
      index 5c7ffd3..1a11160 100644
      class Failure: 
      353353                    try:
      354354                        postMortem(exc[2])
      355355                    except bdb.BdbQuit:
       356                        print "Hi There"
      356357                        pass
       358                    else:
       359                        print "Hi Here"
      357360
      358361            self._realInit(exc_value, exc_type, exc_tb, captureVars)
      359362
  1. Please use reflect.safe_str, rather than hand coding it (in _debugInit).

Otherwise, this looks good. Please resubmit for review after addressing the above points.

comment:19 Changed 4 years ago by Julian Berman

Hey. Thanks.

I switched to safe_str.

For 1, if you run:

from twisted.trial import unittest

class TestFoo(unittest.TestCase):
    def test_thing(self):
        foo = 12

with trial --debug and get to the test body, and then quit (with q), you will see that a {{{bdb.BdbQuit}} exception is raised and gets all the way out of the debugger.

Failure's startDebugMode then will kick in, and you get

Jumping into debugger for post-mortem of exception ''
> /usr/local/Cellar/python/2.7.5/Frameworks/Python.framework/Versions/2.7/lib/python2.7/bdb.py(68)dispatch_line()
-> if self.quitting: raise BdbQuit

Which you will have to quit a second time.

Apparently though what I had doesn't fix it since you get that even with the except there, so I have to investigate when I'm more awake.

comment:20 Changed 4 years ago by Tom Prince

Without doing any investigating, that seems like it isn't the post-mortem triggering that, but rather the other debugger bit.

With:

from twisted.trial.unittest import TestCase
from twisted.python.failure import Failure

class Test(TestCase):
    def test_test(self):
        try:
            raise Exception
        except:
            Failure()

If I 'c' from the first prompt, which continues until the post-mortem is activated. Quitting from there doesn't appear to lead to an exception.

comment:21 Changed 4 years ago by julian

Author: tomprincejulian, tomprince
Branch: branches/failure-select-debugger-5795branches/failure-select-debugger-5795-2

(In [38961]) Branching to failure-select-debugger-5795-2.

comment:22 Changed 4 years ago by Julian Berman

Branch: branches/failure-select-debugger-5795-2branches/failure-select-debugger-5795
Note: See TracTickets for help on using tickets.