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

[Julian Berman]

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.

[Glyph]

#6276 was a duplicate of this.

[Jean-Paul Calderone]

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

[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.

[Julian Berman]

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.

[Julian Berman]

Whoops. Failing test. Fixed.

[Tom Prince]

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

[Tom Prince]

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

Refs: #5795

[Tom Prince]

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.

[Julian Berman]

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).

[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.

[Tom Prince]

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

Refs: #5795

[Tom Prince]

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

Refs: #5795

[Tom Prince]

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.

[Julian Berman]

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 :)

[Tom Prince]

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

   @@ -353 +353 @@
                        try:
                            postMortem(exc[2])
                        except bdb.BdbQuit:
   +                        print "Hi There"
                            pass
   +                    else:
   +                        print "Hi Here"
    
                self._realInit(exc_value, exc_type, exc_tb, captureVars)
    

2. 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.

[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.

[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.

[julian]

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