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

Monkey patch traceback.TracebackException to support MultiError #347

Merged
merged 1 commit into from Nov 28, 2017

Conversation

Projects
None yet
2 participants
@nmalaguti
Contributor

nmalaguti commented Nov 6, 2017

This monkey patches traceback.TracebackException on import. It overrides TracebackException.__init__() to extract any inner exceptions from MultiErrors and overrides TracebackException.format() to include the details of the embedded exceptions.

This deprecates trio.format_exception as it is no longer needed and is now an alias for traceback.format_exception.

Documentation is included detailing some examples where this would be useful e.g. logging.

See gh-305.

@nmalaguti nmalaguti force-pushed the nmalaguti:feature/monkeypatch-traceback branch from 5462841 to f775909 Nov 6, 2017

@codecov

This comment has been minimized.

codecov bot commented Nov 6, 2017

Codecov Report

Merging #347 into master will decrease coverage by <.01%.
The diff coverage is 98.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
- Coverage   99.24%   99.24%   -0.01%     
==========================================
  Files          87       87              
  Lines       10397    10436      +39     
  Branches      729      728       -1     
==========================================
+ Hits        10319    10357      +38     
  Misses         61       61              
- Partials       17       18       +1
Impacted Files Coverage Δ
trio/_toplevel_core_reexports.py 100% <ø> (ø) ⬆️
trio/_core/tests/test_multierror.py 100% <100%> (ø) ⬆️
trio/_core/_multierror.py 99.4% <95.83%> (-0.6%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3edfafe...dd68087. Read the comment docs.

@python-trio python-trio deleted a comment from codecov bot Nov 6, 2017

@@ -6,6 +6,9 @@
from contextlib import contextmanager
import attr
import logging

This comment has been minimized.

@nmalaguti

nmalaguti Nov 6, 2017

Contributor

I don't think this import is used at all anymore. I should remove it.

This comment has been minimized.

@njsmith
# format_exception's semantics for limit= are odd: they apply separately to
# each traceback. I'm not sure how much sense this makes, but we copy it
# anyway.
@deprecated("0.2.0", issue=305, instead="traceback.format_exception")

This comment has been minimized.

@nmalaguti

nmalaguti Nov 6, 2017

Contributor

I wasn't sure what issue number to use for this. I used #305 but it doesn't really address the deprecation. Should I make an issue specifically for this?

This comment has been minimized.

@njsmith

njsmith Nov 11, 2017

Member

You can also use the PR number if it makes more sense - which in this case it probably does, I agree.

This comment has been minimized.

@njsmith

njsmith Nov 11, 2017

Member

(This works because github doesn't fully distinguish between PRs and issues – they use the same number space, and the URL for "issue 347" redirects to the PR page if 347 is a PR.)

chunks += _format_exception_multi(
seen, type(v), v, v.__traceback__, limit=limit, chain=True
if isinstance(exc_value, MultiError):
embedded = []

This comment has been minimized.

@nmalaguti

nmalaguti Nov 6, 2017

Contributor

There is no explicit seen behavior here anymore to log duplicate exceptions. The behavior of TracebackException is just to omit it if it has been seen.

Is that okay?

@nmalaguti nmalaguti requested a review from njsmith Nov 6, 2017

@njsmith

From gitter:

how important is it that a duplicate exception (exc.__cause__ == exc) be printed as a duplicate instead of just omitted?

The current code is really careful about duplicate exceptions, because when I was first writing the MultiError code I had tons of bugs that caused weird reference loops, so I had to make the pretty-printer bulletproof just to figure out what was going on :-). This is probably less important now. It is probably still more important for us than for the stdlib version, because for them the only way you can get a duplicate is if you have something like A.__context__ is A, which is a weird bug and arbitrarily cutting off the display seems ok? For us we can have things like MultiError([A, B]) where A.__context__ is B.__context__, and I feel like here it's a little more confusing if B is just randomly missing its context without any clue that something has happened (especially if A and B are in different parts of the tree).

I guess one approach would be to have the capture code only skip loops, but not throw away "horizontal" duplicates. (Something like, when we recurse we pass through a seen set, but we make a separate copy of it for each entry in a MultiError.) And then on output we can filter more? Or we could define a special placeholder object for "duplicate" that we use when capturing? Can you give some estimate of how hard it would be to keep this?

lookup_lines=True,
capture_locals=False,
_seen=None
):

This comment has been minimized.

@njsmith

njsmith Nov 11, 2017

Member

I think we want an

if _seen is None:
    _seen = set()

here before we call anything else.

seen, type(v), v, v.__traceback__, limit=limit, chain=True
if isinstance(exc_value, MultiError):
embedded = []
for exc in exc_value.exceptions:

This comment has been minimized.

@njsmith

njsmith Nov 11, 2017

Member

Do we want an if exc not in _seen here? I guess it's needed if we want to be absolutely bulletproof. But the only case where it's absolutely necessary is when you have a MultiError that (transitively) embeds itself, which is ... not something that should ever happen. MultiError.__repr__ probably crashes in this case too. So maybe we don't need to worry about it...

'use traceback.format_exception instead' in record[0].message.args[0]
def test_logging(caplog):

This comment has been minimized.

@njsmith

njsmith Nov 11, 2017

Member

Oh excellent, I was going to ask you to copy this test over from the other PR but I see you're way ahead of me :-)

@nmalaguti

This comment has been minimized.

Contributor

nmalaguti commented Nov 12, 2017

I guess one approach would be to have the capture code only skip loops, but not throw away "horizontal" duplicates. (Something like, when we recurse we pass through a seen set, but we make a separate copy of it for each entry in a MultiError.)

Just putting down some thoughts to make sure I understand what each part of the multierror actually represents.

MultiError traceback: This is the stack from where the MultiError was created to where it was handled.

MultiError context or cause: This is any exception that was being handled when the MultiError was created (like if there was a nursery in an except block).

By using the stdlib traceback formatting code, this will work as desired and there should be no duplicates that need to be explicitly printed.

MultiError sub-exceptions: Each want to be printed separately. If there are duplicates, especially in different parts of the "tree", we probably want to mention that instead of silently omitting them.

This matches with what you said about "horizontal" duplicates and using a copy of our _seen at that point for each. I say a copy rather than an empty seen on the off chance that somehow the context of the MultiError we are handling appears as a cause or context of the sub exception, in which case it would be omitted.

In this way, all sub-exceptions will be printed with a reasonable stack of causes, and there may be duplicates in those stacks. In order to re-use the existing TracebackException::format code, we won't be able to mention that the duplicates are duplicates, but as long as the stacks are there it should be okay.

@nmalaguti nmalaguti force-pushed the nmalaguti:feature/monkeypatch-traceback branch from f775909 to bfeca05 Nov 12, 2017

@nmalaguti

This comment has been minimized.

Contributor

nmalaguti commented Nov 12, 2017

@njsmith The only open question left is whether it is important to explicitly mention that we have printed a duplicate exception or if simply printing it is sufficient.

If we want to be explicit that it is a duplicate, we'll need to copy the code from TracebackException::__init__ and print "duplicate" when an exception is in _seen instead of omitting it. I'm not a huge fan of copying the code, but it isn't a lot. I'm also not sure how important it is to know that the exception is the same one, rather than just looks the same.

Monkey patch traceback.TracebackException to support MultiError
This monkey patches `traceback.TracebackException` on import. It
overrides `TracebackException.__init__()` to extract any inner
exceptions from `MultiError`s and overrides
`TracebackException.format()` to include the details of the embedded
exceptions.

This deprecates `trio.format_exception` as it is no longer needed and
is now an alias for `traceback.format_exception`.

Documentation is included detailing some examples where this would be
useful e.g. logging.

See gh-305.

@nmalaguti nmalaguti force-pushed the nmalaguti:feature/monkeypatch-traceback branch from e3e4f75 to dd68087 Nov 14, 2017

@njsmith

This comment has been minimized.

Member

njsmith commented Nov 28, 2017

Sorry for leaving you hanging on this! I think this is looking good, and if we decide we want to tweak it more, we can always do that later :-). Thanks!

@njsmith njsmith merged commit 399b378 into python-trio:master Nov 28, 2017

2 of 4 checks passed

codecov/patch 98.48% of diff hit (target 99.24%)
Details
codecov/project 99.24% (-0.01%) compared to 3edfafe
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

njsmith added a commit to njsmith/trio that referenced this pull request Dec 25, 2017

njsmith added a commit to njsmith/trio that referenced this pull request Dec 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment