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.

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

Prevent an exception escape on unusual classes. #25

Closed
wants to merge 2 commits into from

Conversation

gpshead
Copy link

@gpshead gpshead commented Oct 8, 2021

cheap_repr() is called by snoop on lines of code where class instances
that define their own dunder methods like __getattr__ may be in an
inconsistent state and thus raise an unusual exception.

wrapt.ObjectProxy is one such example. It is used by astroid. So
if you try and snoop the execution of part of pylint, it blows up
due to snoop without this patch.

This could be caught and handled in snoop itself, but it seems like
the point of cheap_repr is to be safe from exceptions?

cheap_repr() is called by snoop on lines of code where class instances
that define their own dunder methods like __getattr__ may be in an
inconsistent state and thus raise an unusual exception.

`wrapt.ObjectProxy` is one such example.  It is used by `astroid`.  So
if you try and `snoop` the execution of part of `pylint`, it blows up
due to snoop without this patch.

This could be caught and handled in `snoop` itself, but it _seems_ like
the point of `cheap_repr` is to be safe from exceptions?
Comment on lines +188 to +196
try:
x_cls = getattr(x, '__class__', type(x))
except Exception: # WAT really? yep! i.e. wrapt.ObjectProxy
# Otherwise https://github.com/GrahamDumpleton/wrapt/blob/1.13.1/src/wrapt/wrappers.py#L209
# causes snooped code to crash during init of this class before it is
# in a consistent state for its __getattr__ method to be called. snoop
# uses cheap_repr in a tracing hook on every line and does not expect
# an exception.
return _try_repr(repr, type(x))
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
try:
x_cls = getattr(x, '__class__', type(x))
except Exception: # WAT really? yep! i.e. wrapt.ObjectProxy
# Otherwise https://github.com/GrahamDumpleton/wrapt/blob/1.13.1/src/wrapt/wrappers.py#L209
# causes snooped code to crash during init of this class before it is
# in a consistent state for its __getattr__ method to be called. snoop
# uses cheap_repr in a tracing hook on every line and does not expect
# an exception.
return _try_repr(repr, type(x))
try:
x_cls = getattr(x, '__class__')
except Exception:
x_cls = type(x)

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately x_cls = type(x) in this situation does not work in the real world scenario I encountered this in.

    def test_wrapt_objectproxy(self):
        import wrapt
        class Uhoh(wrapt.ObjectProxy):
            def __init__(self):
                cheap_repr(self)
>       Uhoh()

tests/test_cheap_repr.py:269: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/test_cheap_repr.py:268: in __init__
    cheap_repr(self)
cheap_repr/__init__.py:204: in cheap_repr
    return _try_repr(func, x, helper)
cheap_repr/__init__.py:217: in _try_repr
    return func(x, *args)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

x = <[ValueError('wrapper has not been initialized') raised in repr()] Uhoh object at 0x7f0500c153c0>, helper = <cheap_repr.ReprHelper object at 0x7f0500c15180>

    @register_repr(object)
    @maxparts(60)
    def repr_object(x, helper):
>       s = repr(x)
E       ValueError: wrapper has not been initialized

I've updated the test to pull in wrapt to demonstrate that. My little test apparently wasn't convoluted enough to demonstrate that on its own.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, but this is now a different problem. It's always going to be possible for repr to raise an exception, but at least now that exception is caught by _try_repr. In this case the exception bubbles up from repr_object, probably because there's a test that calls raise_exceptions_from_default_repr(). In general you should find that the exception is suppressed, and registering a custom repr for Uhoh should also work even if it raises an exception.

Copy link
Owner

Choose a reason for hiding this comment

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

@gpshead ping!

Copy link
Author

Choose a reason for hiding this comment

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

sorry, i'm no longer using this code so i'm unlikely to put further work into this PR. i'll close it, someone else can take it over if they see fit.

@gpshead gpshead closed this Jun 17, 2022
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.

None yet

2 participants