traceback: add a new thin class storing a traceback without storing local variables

[gvanrossum]

BPO	17911
Nosy	@gvanrossum, @rhettinger, @terryjreedy, @ncoghlan, @pitrou, @vstinner, @rbtcollins, @ned-deily, @ericsnowcurrently, @vadmium, @1st1, @Martiusweb
Files	traceback.patch traceback2.patch: Better documentation, fixed edge cases linecache_1.patch linecache_2.patch frame_1.patch tb_stats_1.patch issue17911-1.patch issue17911-2.patch issue17911-3.patch issue17911-4.patch qualname_in_exceptions.patch: Prints qualnames in function call exceptions. issue17911-5.patch issue17911-6.patch issue-19711-8.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2015-03-16.22:17:38.516>
created_at = <Date 2013-05-05.21:31:26.143>
labels = ['type-feature', 'library', 'expert-asyncio']
title = 'traceback: add a new thin class storing a traceback without storing local variables'
updated_at = <Date 2016-06-29.05:00:26.293>
user = 'https://github.com/gvanrossum'

bugs.python.org fields:

activity = <Date 2016-06-29.05:00:26.293>
actor = 'martin.panter'
assignee = 'none'
closed = True
closed_date = <Date 2015-03-16.22:17:38.516>
closer = 'rbcollins'
components = ['Library (Lib)', 'asyncio']
creation = <Date 2013-05-05.21:31:26.143>
creator = 'gvanrossum'
dependencies = []
files = ['30895', '30901', '37700', '37769', '37770', '37771', '37782', '37862', '37870', '38007', '38034', '38324', '38325', '38335']
hgrepos = []
issue_num = 17911
keywords = ['patch']
message_count = 77.0
messages = ['188467', '188530', '188548', '188554', '188556', '188616', '188620', '188648', '188719', '192672', '192690', '192728', '192909', '193272', '193294', '193296', '193333', '220381', '220390', '231743', '231792', '233895', '233995', '234297', '234298', '234299', '234319', '234338', '234712', '234740', '234741', '234765', '234769', '234777', '234781', '234782', '234788', '234795', '234796', '234797', '234804', '234812', '234820', '235365', '235428', '235444', '235446', '235455', '235456', '235472', '235480', '235483', '235517', '235569', '235570', '235574', '235578', '235658', '235720', '235872', '237166', '237217', '237220', '237224', '237225', '237228', '237229', '237230', '237231', '237232', '237233', '237237', '237339', '237842', '238169', '238248', '269464']
nosy_count = 16.0
nosy_names = ['gvanrossum', 'rhettinger', 'terry.reedy', 'ncoghlan', 'pitrou', 'vstinner', 'rbcollins', 'ned.deily', 'python-dev', 'eric.snow', 'martin.panter', 'mahmoud', 'yselivanov', 'adaptivelogic', 'martius', 'xonatius']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = None
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue17911'
versions = ['Python 3.5']

[gvanrossum]

For Tulip I may have to extract tracebacks and store the extracted data, in order to avoid cycles created by the frames referenced in the traceback. I'll be extracting the traceback every time an exception is raised, and in most cases I won't be using the extracted info, because usually the exception is caught (or logged) by user code. But it's very important to ensure that if the user code doesn't catch or log it, I can log a traceback, and I won't know that this is the case until a destructor is called, which may be quite a bit later. (Reference: http://code.google.com/p/tulip/source/browse/tulip/futures.py#38)

Unfortunately it looks like the public APIs do a lot more work than needed. Ideally, there'd be something similar to _extract_tb_or_stack_iter() that doesn't call linecache.getline() -- it should just return triples of (filename, lineno, functionname), and enough structure to tell apart the __cause__, __context__, and __traceback__ (the first two possibly repeated). Given this info it would be simple enough to format and log the actual traceback, and storing just this would take less space and time than computing the lines of the fully-formatted traceback.

[pitrou]

I think instead we may want to add a finalize() or close() method on frame objects which would clear all local variables (as well as dereference the globals dict, perhaps), after having optionally run a generator's close() method (if the frame belongs to a generator).

If I'm not mistaken, it should allow breaking reference cycles, and remove the need for complex traceback processing, which Twisted currently also does: http://twistedmatrix.com/trac/browser/trunk/twisted/python/failure.py#L89

Note that generator cleanup through the frame has a patch in bpo-17807.

[gvanrossum]

I'm not snubbing my nose at something that breaks these types of cycles more easily, but I still think that the abstractions offered by the traceback module could be improved.

[gvanrossum]

On Mon, May 6, 2013 at 6:06 AM, Antoine Pitrou <report@bugs.python.org> wrote:

Note that generator cleanup through the frame has a patch in bpo-17807.

Sadly that doesn't help with my issue. I applied path gen3.patch and
ran various versions of the code I have. There's still something
holding on to the Task or the exception.

[pitrou]

On Mon, May 6, 2013 at 6:06 AM, Antoine Pitrou
<report@bugs.python.org> wrote:
> Note that generator cleanup through the frame has a patch in
> bpo-17807.

Sadly that doesn't help with my issue.

It won't. It's just a building block for the change I've proposed here.

[ncoghlan]

Antoine - I like your idea, but I think it's a separate issue. I agree with Guido that exposing some lower level non-formatting APIs in the traceback module would be helpful.

I see Guido's suggestion here as similar to the change we just made in the dis module to expose a dis.get_instructions iterator. That change makes it much easier to work with the disassembler output programmatically, whereas the module was previously too focused on displaying text with a specific format.

My current thoughts: define a "TracebackSummary" with the following fields:

exc_type
exc_repr
stack_summary
cause
context

stack_summary would be a list of (filename, lineno, functionname) triples as Guido suggested (probably a named tuple)

cause and context would be either None or a reference to an appropriate TracebackSummary object

Basically, the summary should contain all of the information needed to create the formatted traceback output, without actually doing any formatting (aside from calling repr() on the exception)

[gvanrossum]

That sounds great, except I'd expect the exception type and str(), since a formatted traceback uses the str() of the exception, not its repr().

Personally I don't think the tuples need to be named, but I won't hold up progress either. :-)

[pitrou]

You may want two pieces of stack: the piece of stack that is above and including the catch point, and the piece of stack that is below it. The former is what gets traditionally printed in tracebacks, but the latter can be useful as well (see bpo-1553375 for a related feature request).

[pitrou]

Ok, I've spinned off the frame clearing suggestion to bpo-17934.

[adaptivelogic]

I've been looking into this as an easy piece to bite off. If I understand Guido correctly, he'd like to defer or suppress the linecache call when getting the tb summary. The problem with deferring is that you need to access f_globals for the loader to work correctly when the module source is a non-file import source. If we keep a reference to f_globals for each line in the traceback, we can defer this to later (ideally we'd just keep the __loader__ value, but that would require changing the linecache interface as well).

My inclination would be to have another keyword argument to _extract_tb_or_stack_iter (terse=False or verbose=True - either style works). In terse mode, no source lines would be available, and the formatted output would be the same as if the source wasn't available at all. This would work, although the traceback module is structured so that I'd need to pass it through quite a few wrapped iterator calls.

I'm not sure how free a hand I have when it comes to refactoring the internal implementation. I'm not fond of the extractor callbacks - I'd prefer a generator-based approach on the lines of:

def _tb_iter(tb, limit):
    i = 0
    while tb is not None:
        if limit is not None and limit < i:
            break
        yield tb.tb_frame, tb.tb_lineno
        tb = tb.tb_next
        i += 1

def _extract_frame_iter(frames, terse=False):
    ...
    for frame, lineno in frames:
    ...

[gvanrossum]

In terms of how much freedom you have about changing the internal, I'd check how long ago they were changed. "Internal" APIs that have been stable for many versions tend to have illicit external uses -- but internal APIs that were introduced recently (e.g. in 3.2) are usually safe to use -- nobody is going to make too much of a stink if you break their code.

As for saving f_globals, if you're going to save an extra pointer anyways, why not just save the frame pointer?

[pitrou]

If we keep a reference to f_globals for each line in the traceback, we
can defer this to later (ideally we'd just keep the __loader__ value,
but that would require changing the linecache interface as well).

Or you could add a *new* linecache interface.
Keeping __loader__ sounds much less hackish than keeping f_globals to me.

[adaptivelogic]

After thinking about it, I decided to defer instead of suppress line fetching. Suppressing would still give a traceback later, but not the same as the original.

extract_exception is where the meat is. There's a slight quirk in how context works - if cause is set to something else than the actual exception, context still gets set even though it won't be printed. This is to enable later analysis. However, if you explicitly 'raise Exception from None' neither of them will be set - the formatting code would have issues otherwise.

I'm not too happy about the code duplication between the format_exception method and the formatting that works on exceptions directly, but there are plenty of tests to ensure that the output is identical.

Feel free to dispute my naming choices. I'm admittedly influenced by twisted.

[pitrou]

I think ExceptionSummary could be the visible API, instead of adding an indirection through a separate global function.
Also, I don't think having the "exc_traceback" list of tuples is future-proof. What if we want to add some information to each traceback entry, or refactor it to take __loader__ into account?
Instead, ExceptionSummary could expose the desired operations (e.g. iterate over traceback lines and the associated source code lines) without being constrained by some implementation details.

[adaptivelogic]

I was trying to stay with the established pattern of the existing methods. There are two unrelated issues to solve here - deferring linecache access, and the extract_exception functionality.

When it comes to deferral, we could wrap each line in a different class than tuple, but this constitutes a public API change (and this is a *very* widely used library). Even changing to a namedtuple would cause a lot of grief, since we'd break lots of doctests, and subclassing tuple is a lot of effort for little benefit. If we only do it for the deferred usage, it would "only" be inconsistent. I suppose we could have a completely separate way of doing things for ExceptionSummary, but again, inconsistent, and I think if one extract_xxx method provides the functionality, users would expect it to be available in the others.

For ExceptionSummary, the same summary instance is used more than once if you have a cause set, so object creation had to be separated from the graph population. Either this is done in a constructor function or we would need some obscure tricks with the class kwargs. This way, there's a separation of concerns - the class wraps *one* exception, and the function creates a bunch and links them together as needed.

[pitrou]

When it comes to deferral, we could wrap each line in a different
class than tuple, but this constitutes a public API change (and this
is a *very* widely used library). Even changing to a namedtuple
would cause a lot of grief, since we'd break lots of doctests, and
subclassing tuple is a lot of effort for little benefit. If we only
do it for the deferred usage, it would "only" be inconsistent. I
suppose we could have a completely separate way of doing things for
ExceptionSummary, but again, inconsistent, and I think if one
extract_xxx method provides the functionality, users would expect it
to be available in the others.

YMMV, but I think we should go for inconsistency here. The other APIs
in the traceback module are low-level and clearly limited by the type
of objects they return.

This feature request is a good opportunity to design something a little
more future-proof. I'd love to know what other developers/contributors
think here.

[ncoghlan]

For dis, we introduced a new rich bytecode introspection API. Ditto for
inspect.Signature. I think it makes sense to treat this as a completely new
traceback introspection API and ignore the low level details of the legacy
API.

[rhettinger]

I think it makes sense to treat this as a completely new
traceback introspection API and ignore the low level details
of the legacy API.

That would likely be the cleanest approach.

[vstinner]

While trying to fix a similar issue for the asyncio project, I wrote the following code:
https://bitbucket.org/haypo/misc/src/ce48d7b3ea1d223691e496e41aca8f5784671cd5/python/suppress_locals.py?at=default

I was not aware that a similar approach (attached traceback2.patch) was already implemented.

Asyncio issues:
https://code.google.com/p/tulip/issues/detail?id=155
https://code.google.com/p/tulip/issues/detail?id=42

See also the Python issue bpo-20032.

[ncoghlan]

Belatedly looking at this again: one thing I noticed is that the summary currently loses info if __cause__ is set.

It would be better to follow the PEP 409/415 model and report the "__suppress_context__" flag as part of the summary, rather than dropping the context information.

That allows the decision on whether or not to show the context information to be deferred to display time, rather than discarding it eagerly.

[rbtcollins]

I'll put something together.

[rbtcollins]

w.r.t. a new linecache interface, it looks like we need two attributes from f_globals: __name__ and __loader__, so that we can eventually call __loader__.get_source(name).

One small change (to let me focus on traceback) would be to add another kw argument to the existing calls that take module_globals, called e.g. get_source, which would be a simple callable (source = get_source()). For the deferred linecache situation, we'd then create partial(f_globals.__loader__.get_source, f_globals.__name__) and keep that around until we need the source. We could even abstract that out into a function in linecache to keep the knowledge in one place.

Another way to tackle this would be to add a new function to linecache that lazily seeds the cache: it would stash the get_source method and the name against the filename, and when getline[s] is called actually invoke it.

I think the second way is a bit nicer myself. What do folk think?

[rbtcollins]

Ok, here's a draft patch for linecache. Next up, poking around the new TB API.

[rbtcollins]

One thing I noticed while working on this, traceback today stats each included file once per frame per call into e.g. format_list. This might actually account for quite some of the overhead. I'll include an optimisation for this in my new API. http://paste.openstack.org/show/158714/
http://paste.openstack.org/show/158715/

[pitrou]

Le 06/02/2015 09:34, Nick Coghlan a écrit :

It's genuinely user hostile, as the "from_exc_tuple" version is entirely
redundant, and folks using pydoc to discover the API won't know they
"shouldn't" use the constructor directly.

Using pydoc isn't generally a successful way of discovering an API. It
shows you too many details that don't matter. Besides our docstrings are
much terser and less informative than the Sphinx doc.

pydoc is useful mostly as a Python-specific "man" command: you know
something exists, just want to remember the details.

[ncoghlan]

dir() will get me TracebackException as a name, help(traceback.TracebackException) will get me its signature. IDEs with autocomplete and signature tooltips will do the same.

There is nothing in those usage sequences to say "don't use __init__, use this redundant class method with the same signature instead".

I agree providing a "directly from an exception" constructor is essential for cases where you're working with a caught exception at the Python level. I don't agree with the idea of duplicating the required low level API under a different name so it doesn't *look* like a lower level API in the documentation.

[xonatius]

As suggested adding patch from http://bugs.python.org/issue2786 which prints qualnames in function call exceptions.

e.g:

>>> class A:
...     def __init__(self):
...         pass
>>> A(1)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: A.__init__() takes 1 positional argument but 2 were given

[rbtcollins]

Sorry xonatius I wasn't clear: AFAICT your patch is going to require changes to the traceback tests, and this issue is changing the implementation substantially: I was suggesting that you make sure your patch applies on top of this issue, not that you merge them :)

[rbtcollins]

Nick, Antoine - I'm now stuck between you. Options going forward:

• I can JFDI realising you won't both be happy :)
• you two can reach consensus!

I could cripple __init__ by switching to __new__, but I think thats massive overcomplication and not needed.

I can of course add more text to the pydoc prose saying 'do not use __init__' if that would address Nicks concern.

[ncoghlan]

Escalating to python-dev for API design feedback is the usual path forward
if we reach an impasse in the tracker comments.

I'll make one more attempt at persuading Antoine here though: the fact that
we're being tempted to add "do not use this API the way you would normally
expect to use a Python API, even though it works exactly as you might
expect" to the docs is a big red flag for me :)

There's a split between the "low level API that exposes implementation
details" (the exception state triple) and "high level API that hides them"
in the current patch, and Antoine is entirely correct that we previously
omitted the latter. It doesn't follow for me that we should also hide the
fact that the higher level convenience API is implemented in terms of the
lower level more implementation oriented one.

[pitrou]

You can also switch back to a single constructor, taking either an
exception instance or an exception triple. That way everyone is happy -
except perhaps if you are ideologically opposed to polymorphic
signatures :-)

Switching to python-dev would probably be overkill for this.

[rbtcollins]

I'm idealogically opposed to polymorphic interpretation of args :)

Antoine, will you be ok with one __init__ and one classmethod?

[mahmoud]

Hey all, great to see this being worked on so diligently for so long. Having worked in this area for a while (at home and at PayPal), we've got a few learnings to share:

1. linecache is textbook not-threadsafe. For example, https://hg.python.org/cpython/file/default/Lib/linecache.py#l38

For a lightweight traceback wrapper to be concurrency-friendly, we've had to catch KeyErrors, like so: https://github.com/mahmoud/boltons/blob/master/boltons/tbutils.py#L115

It's kind of a blanket approach, but maybe we could make a separate issue and help out with a linecache refresh?

2. We use something like (filename, lineno) in our DeferredLine class, but for very lightweight areas (e.g., greenlet creation) we just save a reference to the code object, as the additional attribute accesses do end up showing up in the profiles.

3. Generally we've found the APIs in TracebackInfo here to be pretty sufficient/functional:

https://github.com/mahmoud/boltons/blob/master/boltons/tbutils.py#L134

Let me know if you've got any questions on that, and keep up the good work!

[rbtcollins]

@mahmoud thanks! I had a quick look and the structural approach we've taken is a bit different. What do you think of the current patch here (bpo-17911-4.patch) ?

[rbtcollins]

Ok, all changes applied, lets see how this looks to folk.

[python-dev]

New changeset 73afda5a4e4c by Robert Collins in branch 'default':
Issue bpo-17911: traceback module overhaul
https://hg.python.org/cpython/rev/73afda5a4e4c

[python-dev]

New changeset 7cea10917f40 by Robert Collins in branch 'default':
Fix brownbag in bpo-17911 commit
https://hg.python.org/cpython/rev/7cea10917f40

[ned-deily]

With 7cea10917f40 applied, multiple tests are now failing on most buildbots. For example, on OS X 10.10, test_cgitb test_code_module test_decimal:

test_blanks (test.test_cgitb.TestCgitb) ... ok
test_fonts (test.test_cgitb.TestCgitb) ... ok
test_html (test.test_cgitb.TestCgitb) ... ERROR
test_syshook_no_logdir_default_format (test.test_cgitb.TestCgitb) ... ok
test_syshook_no_logdir_text_format (test.test_cgitb.TestCgitb) ... ok
test_text (test.test_cgitb.TestCgitb) ... ERROR

test_banner (test.test_code_module.TestInteractiveConsole) ... ok
test_cause_tb (test.test_code_module.TestInteractiveConsole) ... ERROR
test_console_stderr (test.test_code_module.TestInteractiveConsole) ... ok
test_context_tb (test.test_code_module.TestInteractiveConsole) ... ERROR
test_ps1 (test.test_code_module.TestInteractiveConsole) ... ok
test_ps2 (test.test_code_module.TestInteractiveConsole) ... ok
test_syntax_error (test.test_code_module.TestInteractiveConsole) ... ERROR
test_sysexcepthook (test.test_code_module.TestInteractiveConsole) ... ERROR

---

1 items had failures:
1 of 4 in decimal.Context.create_decimal_from_float

See also, for example, the Debian buildbot where, in addition, test_fractions failed:

http://buildbot.python.org/all/builders/AMD64%20Debian%20PGO%203.x/builds/140/steps/test/logs/stdio

[rbtcollins]

Thats really strange, I did a ./python -m test run before committing - the brown bag was due to me running with the patch for 22936 also applied. Looking into the failures reported now.

[rbtcollins]

Ok, cgitb - its passing a string instead of an exception type into format_exception - something that was never supported - it only works by accident AFAICT, because the old format code was ignoring the etype - it was deriving the type from the value. Thats easy to accomodate in the compat code.

[rbtcollins]

Also apologies - ned told me on IRC that python -m test -uall is needed, not just -m test. Doh.

[vstinner]

There are failures on buildbot.
http://buildbot.python.org/all/builders/AMD64%20Ubuntu%20LTS%203.x/builds/5651/steps/test/logs/stdio
http://buildbot.python.org/all/builders/AMD64%20Windows7%20SP1%203.x/builds/5783/steps/test/logs/stdio
http://buildbot.python.org/all/builders/PPC64%20PowerLinux%203.x/builds/3109/steps/test/logs/stdio

Examples:

======================================================================
ERROR: test_cause_tb (test.test_code_module.TestInteractiveConsole)
----------------------------------------------------------------------

Traceback (most recent call last):
AttributeError

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 91, in runcode
    exec(code, self.locals)
  File "<console>", line 1, in <module>
ValueError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/python/3.x.langa-ubuntu/build/Lib/test/test_code_module.py", line 85, in test_cause_tb
    self.console.interact()
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 245, in interact
    more = self.push(line)
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 267, in push
    more = self.runsource(source, self.filename)
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 75, in runsource
    self.runcode(code)
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 95, in runcode
    self.showtraceback()
  File "/opt/python/3.x.langa-ubuntu/build/Lib/code.py", line 144, in showtraceback
    for value, tb in traceback._iter_chain(*ei[1:]):
AttributeError: module 'traceback' has no attribute '_iter_chain'

======================================================================
ERROR: test_html (test.test_cgitb.TestCgitb)
----------------------------------------------------------------------

Traceback (most recent call last):
  File "/home/shager/cpython-buildarea/3.x.edelsohn-powerlinux-ppc64/build/Lib/test/test_cgitb.py", line 23, in test_html
    raise ValueError("Hello World")
ValueError: Hello World

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/home/shager/cpython-buildarea/3.x.edelsohn-powerlinux-ppc64/build/Lib/test/test_cgitb.py", line 27, in test_html
    html = cgitb.html(sys.exc_info())
  File "/home/shager/cpython-buildarea/3.x.edelsohn-powerlinux-ppc64/build/Lib/cgitb.py", line 189, in html
    ''.join(traceback.format_exception(etype, evalue, etb)))
  File "/home/shager/cpython-buildarea/3.x.edelsohn-powerlinux-ppc64/build/Lib/traceback.py", line 109, in format_exception
    etype, value, tb, limit=limit).format(chain=chain))
  File "/home/shager/cpython-buildarea/3.x.edelsohn-powerlinux-ppc64/build/Lib/traceback.py", line 433, in __init__
    if exc_type and issubclass(exc_type, SyntaxError):
TypeError: issubclass() arg 1 must be a class

[rbtcollins]

code module is using a _function from within the traceback module to filter out a frame - we can do that with the new interface easily (it filters the first tem).

[rbtcollins]

The decimal failure is due to _format_final_exc_line now filtering out 'None' as well, because the string captured values of objects leads to None -> 'None' before we do rendering.

I think in this case its reasonable to change the behaviour (since None itself was already filtered out, its only the special case of a value of 'None' that worked previously).

[rbtcollins]

Fixes for buildbots.

[ned-deily]

Much better, thanks!

[python-dev]

New changeset 648b35f22b91 by Berker Peksag in branch 'default':
Issue bpo-17911: Tweak traceback documentation.
https://hg.python.org/cpython/rev/648b35f22b91

[terryjreedy]

(correct new issue #) Idle is partially disabled in a2 (bpo-23631), with symptoms similar to that of the first example in msg237230. Trivial test: open Idle Shell, enter '1/0'. If this is not fixed before .a3, I think it should be reverted until it is.

[rbtcollins]

Looking at the regression now.

[rbtcollins]

Regression fixed AFAICT, please re-open if not.

[vadmium]

Robert: in bpo-17911-2.patch (and the eventual commit) you added a check for value == 'None' in _format_final_exc_line(). Why was this necessary? I think it is the cause of my bpo-27348 regression:

>>> traceback.format_exception(Exception, Exception("One"), None)
['Exception: One\n']
>>> traceback.format_exception(Exception, Exception("None"), None)
['Exception\n']