protect IPython from bad custom exception handlers #876

Merged
merged 7 commits into from Oct 16, 2011

Conversation

Projects
None yet
2 participants
@minrk
Member

minrk commented Oct 14, 2011

Previously, errors in custom handlers would result in the custom exception
handler's error being printed in lieu of the real exception, and certain cases could cause infinite loops.

Now, if CustomTB fails it is unregistered immediately, and the original TB is also displayed.

IPython's own BdbQuit_IPython_excepthook had an invalid signature, which revealed this issue, and has also been fixed.

test included.

closes #692

minrk added some commits Oct 14, 2011

protect IPython from bad custom exception handlers
Previously, errors in custom handlers would result in the custom exception
handler's error being printed in lieu of the real exception, and certain cases could cause infinite loops.

Now, if CustomTB fails it is unregistered immediately, and the original TB is also displayed.

IPython's own BdbQuit_IPython_excepthook had an invalid signature, which revealed this issue, and has also been fixed.

test included.

closes #692
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 14, 2011

Member

While looking at the same code, I saw the culprit for #690 as well, so I fixed that, too.

Member

minrk commented Oct 14, 2011

While looking at the same code, I saw the culprit for #690 as well, so I fixed that, too.

prevent atexit handlers from generating crash report
register `sys.excepthook = sys.__excepthook__` with atexit on aplication startup,
so it should be the first handler called.  This removes the crash handler.

closes #207
@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 15, 2011

Member

Code looks good, but this simple snippet

from IPython.core.debugger import Tracer
Tracer()

currently exits silently. I've never actually used Tracer() myself, but I'd expect it to activate the debugger in tracing mode, yet it doesn't. Based on the discussion of #692, does this really close the ticket?

Member

fperez commented Oct 15, 2011

Code looks good, but this simple snippet

from IPython.core.debugger import Tracer
Tracer()

currently exits silently. I've never actually used Tracer() myself, but I'd expect it to activate the debugger in tracing mode, yet it doesn't. Based on the discussion of #692, does this really close the ticket?

protect against bad return type of CustomTB
includes test
also expands a few docstrings
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 15, 2011

Member

Tracer() is a callable object that can be invoked to set_trace.

I did find a typo to fix, and added return-type validation, which I just pushed.

This is how Tracer appears to be used, based on the source:

from IPython.core.debugger import Tracer
tracer = Tracer()

try:
    1/0
except:
    tracer()
Member

minrk commented Oct 15, 2011

Tracer() is a callable object that can be invoked to set_trace.

I did find a typo to fix, and added return-type validation, which I just pushed.

This is how Tracer appears to be used, based on the source:

from IPython.core.debugger import Tracer
tracer = Tracer()

try:
    1/0
except:
    tracer()
@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 15, 2011

Member

Question, shouln't we be trapping the quit exception? Trying out the example you show, I'm getting this when I quit:

dreamweaver[~]> python mini.py 
--Return--
None
> /home/fperez/mini.py(7)()
      6 except:
----> 7     tracer()
      8 

ipdb> q
Traceback (most recent call last):
  File "mini.py", line 7, in 
    tracer()
  File "/usr/lib/python2.6/bdb.py", line 50, in trace_dispatch
    return self.dispatch_return(frame, arg)
  File "/usr/lib/python2.6/bdb.py", line 84, in dispatch_return
    if self.quitting: raise BdbQuit
bdb.BdbQuit

I have vague memories of, long ago, having written code in ipdb to handle that case, but maybe it's not in the right place when used in standalone tracing mode...

Member

fperez commented Oct 15, 2011

Question, shouln't we be trapping the quit exception? Trying out the example you show, I'm getting this when I quit:

dreamweaver[~]> python mini.py 
--Return--
None
> /home/fperez/mini.py(7)()
      6 except:
----> 7     tracer()
      8 

ipdb> q
Traceback (most recent call last):
  File "mini.py", line 7, in 
    tracer()
  File "/usr/lib/python2.6/bdb.py", line 50, in trace_dispatch
    return self.dispatch_return(frame, arg)
  File "/usr/lib/python2.6/bdb.py", line 84, in dispatch_return
    if self.quitting: raise BdbQuit
bdb.BdbQuit

I have vague memories of, long ago, having written code in ipdb to handle that case, but maybe it's not in the right place when used in standalone tracing mode...

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 15, 2011

Member

It does handle BdbQuit correctly from IPython. I don't know why it doesn't in regular Python. The exception handler is set when outside IPython here.

Adding a print statement to the excepthook, it is never called.

Member

minrk commented Oct 15, 2011

It does handle BdbQuit correctly from IPython. I don't know why it doesn't in regular Python. The exception handler is set when outside IPython here.

Adding a print statement to the excepthook, it is never called.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 15, 2011

Member

Nevermind, I found it - ipapi.get() is not an acceptable way to check if you are in IPython - it will always return an IPython shell. get_ipython is the way to check if you are currently in IPython.

Member

minrk commented Oct 15, 2011

Nevermind, I found it - ipapi.get() is not an acceptable way to check if you are in IPython - it will always return an IPython shell. get_ipython is the way to check if you are currently in IPython.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 15, 2011

Member

Should I also do something about #636, while I'm cleaning up debugger miscellany?

pydb checks for '-pydb' in sys.argv, but there is no pydb flag anymore. Should I add --pydb as a flag to Shell Apps?

Member

minrk commented Oct 15, 2011

Should I also do something about #636, while I'm cleaning up debugger miscellany?

pydb checks for '-pydb' in sys.argv, but there is no pydb flag anymore. Should I add --pydb as a flag to Shell Apps?

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 15, 2011

Member

Looking into pydb, it would appear that the project is entirely deprecated (files removed from pypi, even), in favor of a rewrite called pydbgr, which is not even installable as far as I can tell, and also appears essentially abandoned.

Member

minrk commented Oct 15, 2011

Looking into pydb, it would appear that the project is entirely deprecated (files removed from pypi, even), in favor of a rewrite called pydbgr, which is not even installable as far as I can tell, and also appears essentially abandoned.

re-enable pydb flag
Note that pydb has been deprecated, and superseded by pydbgr, which may be
abandoned.

It would be preferable if the Pdb class could inherit from pydb or pdb
based on a runtime flag rather than checking sys.argv at the top level.
This at least restores old behavior for pydb users.

closes #636
@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 15, 2011

Member

I re-enabled the pydb flag, so it should work. It doesn't set any config, it just allows the flag to pass through, so debugger.py can make its check. Ultimately this should probably be fixed so the check doesn't need to be run at import-time, but for now, at least, we don't have a never-true invalid flag check.

Member

minrk commented Oct 15, 2011

I re-enabled the pydb flag, so it should work. It doesn't set any config, it just allows the flag to pass through, so debugger.py can make its check. Ultimately this should probably be fixed so the check doesn't need to be run at import-time, but for now, at least, we don't have a never-true invalid flag check.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 15, 2011

Member

This is looking pretty good. Only one note: 09c2492 makes the correct check, but by not instantiating a full ipython, now the ipdb instance comes up without coloring. It would be nice to restore that, though without waiting to fire up a full blown ipython. That would mean parsing the config for colors...

If you think you can take a quick shot at reading the color settings correctly to instantiate the tracer with color active and matching the user's choice, that would be great.

But if it's looking like too much work, don't sweat it. I don't want to hold this PR forever on that little bit of nicety, so I'm happy to just leave an issue open for color support in the tracer and move on as-is. Your call.

Member

fperez commented Oct 15, 2011

This is looking pretty good. Only one note: 09c2492 makes the correct check, but by not instantiating a full ipython, now the ipdb instance comes up without coloring. It would be nice to restore that, though without waiting to fire up a full blown ipython. That would mean parsing the config for colors...

If you think you can take a quick shot at reading the color settings correctly to instantiate the tracer with color active and matching the user's choice, that would be great.

But if it's looking like too much work, don't sweat it. I don't want to hold this PR forever on that little bit of nicety, so I'm happy to just leave an issue open for color support in the tracer and move on as-is. Your call.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 15, 2011

Member

I'm out for the afternoon, but I think it could be a simple call to terminal.ipapp.load_default_config(), so I'll check on that when I get home.

-MinRK

On Oct 15, 2011, at 14:11, Fernando Perezreply@reply.github.com wrote:

This is looking pretty good. Only one note: 09c2492 makes the correct check, but by not instantiating a full ipython, now the ipdb instance comes up without coloring. It would be nice to restore that, though without waiting to fire up a full blown ipython. That would mean parsing the config for colors...

If you think you can take a quick shot at reading the color settings correctly to instantiate the tracer with color active and matching the user's choice, that would be great.

But if it's looking like too much work, don't sweat it. I don't want to hold this PR forever on that little bit of nicety, so I'm happy to just leave an issue open for color support in the tracer and move on as-is. Your call.

Reply to this email directly or view it on GitHub:
#876 (comment)

Member

minrk commented Oct 15, 2011

I'm out for the afternoon, but I think it could be a simple call to terminal.ipapp.load_default_config(), so I'll check on that when I get home.

-MinRK

On Oct 15, 2011, at 14:11, Fernando Perezreply@reply.github.com wrote:

This is looking pretty good. Only one note: 09c2492 makes the correct check, but by not instantiating a full ipython, now the ipdb instance comes up without coloring. It would be nice to restore that, though without waiting to fire up a full blown ipython. That would mean parsing the config for colors...

If you think you can take a quick shot at reading the color settings correctly to instantiate the tracer with color active and matching the user's choice, that would be great.

But if it's looking like too much work, don't sweat it. I don't want to hold this PR forever on that little bit of nicety, so I'm happy to just leave an issue open for color support in the tracer and move on as-is. Your call.

Reply to this email directly or view it on GitHub:
#876 (comment)

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 16, 2011

Member

It's not quite as simple as I thought.

There are two defaults:

  • load_default_config() will load your default profile
  • InteractiveShell.colors.get_default_value() will get the default value from the class, sans config.

It's easy to do the latter, but it turns out the former is less easy (or at least less clean), due to Class heirarchy, as we really use TerminalInteractiveShell, a subclass of InteractiveShell, both having a colors argument (again, colors must eventually be removed from the InteractiveShell object, as they are a purely frontend notion, but that's not relevant here).

So, to match the default resolution of the TerminalInteractiveShell's colors with default config would be:

            cfg = load_default_config()
            try:
                def_colors = cfg.TerminalInteractiveShell.colors
            except AttributeError:
                try:
                    def_colors = cfg.InteractiveShell.colors
                except AttributeError:
                    def_colors = InteractiveShell.colors.get_default_value()

And any changes to inheritance could muck this up.

Member

minrk commented Oct 16, 2011

It's not quite as simple as I thought.

There are two defaults:

  • load_default_config() will load your default profile
  • InteractiveShell.colors.get_default_value() will get the default value from the class, sans config.

It's easy to do the latter, but it turns out the former is less easy (or at least less clean), due to Class heirarchy, as we really use TerminalInteractiveShell, a subclass of InteractiveShell, both having a colors argument (again, colors must eventually be removed from the InteractiveShell object, as they are a purely frontend notion, but that's not relevant here).

So, to match the default resolution of the TerminalInteractiveShell's colors with default config would be:

            cfg = load_default_config()
            try:
                def_colors = cfg.TerminalInteractiveShell.colors
            except AttributeError:
                try:
                    def_colors = cfg.InteractiveShell.colors
                except AttributeError:
                    def_colors = InteractiveShell.colors.get_default_value()

And any changes to inheritance could muck this up.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 16, 2011

Member

In the debugger code, the default colors are hardcoded to NoColor (Tracer has a colors parameter, while Pdb has color_scheme), is it possible that's deliberate and/or desirable? I can easily change them to use InteractiveShell's default. I think using config files should be put off until the debugger code is all updated to be config aware.

Member

minrk commented Oct 16, 2011

In the debugger code, the default colors are hardcoded to NoColor (Tracer has a colors parameter, while Pdb has color_scheme), is it possible that's deliberate and/or desirable? I can easily change them to use InteractiveShell's default. I think using config files should be put off until the debugger code is all updated to be config aware.

@fperez

This comment has been minimized.

Show comment
Hide comment
@fperez

fperez Oct 16, 2011

Member

I guess it was a matter of being over-conservative in the interest of safety: absent a way to detect the actual user's config choices, going with no colors at least will work OK in all terminals, where as the wrong choice of colors for certain backgrounds produces horrible usability.

So my take would be: if it's not really practical right now to get the standalone debugger/tracer to actually pick up the user's config choices, let's default to nocolor. As long as users can insantiate the tracer themselves with color info, that should suffice for more advanced uses. But I think we should maintain the design principle of plain-but-robust beats pretty-but-brittle.

Member

fperez commented Oct 16, 2011

I guess it was a matter of being over-conservative in the interest of safety: absent a way to detect the actual user's config choices, going with no colors at least will work OK in all terminals, where as the wrong choice of colors for certain backgrounds produces horrible usability.

So my take would be: if it's not really practical right now to get the standalone debugger/tracer to actually pick up the user's config choices, let's default to nocolor. As long as users can insantiate the tracer themselves with color info, that should suffice for more advanced uses. But I think we should maintain the design principle of plain-but-robust beats pretty-but-brittle.

@minrk

This comment has been minimized.

Show comment
Hide comment
@minrk

minrk Oct 16, 2011

Member

Okay, then that's how it is now, without changes.

Member

minrk commented Oct 16, 2011

Okay, then that's how it is now, without changes.

fperez added a commit that referenced this pull request Oct 16, 2011

Merge pull request #876 from minrk/customtb
Protect IPython from bad custom exception handlers.  Also ensures that ipdb can be used as a tracing debugger in a manner similar to `pdb.set_trace`, via:

from IPython.core.debugger import Tracer
tracer = Tracer()
# then, call tracer() anywhere in the code to start it up.

@fperez fperez merged commit aa846e3 into ipython:master Oct 16, 2011

mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

Merge pull request #876 from minrk/customtb
Protect IPython from bad custom exception handlers.  Also ensures that ipdb can be used as a tracing debugger in a manner similar to `pdb.set_trace`, via:

from IPython.core.debugger import Tracer
tracer = Tracer()
# then, call tracer() anywhere in the code to start it up.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment