TracebackException or StackSummary.extract with capture_locals=True fail to catch exceptions raised by repr() on value of frame local variable in FrameSummary.__init__.

[moi90]

BPO	43656
Nosy	@rbtcollins, @serhiy-storchaka, @iritkatriel, @moi90, @akulakov
PRs	bpo-43656: Make FrameSummary robust towards un-repr-able values #25062 bpo-43656: Introduce format_locals in traceback #29299

^{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 = None
created_at = <Date 2021-03-29.09:36:49.757>
labels = ['invalid', 'type-feature', '3.8', '3.9', '3.10']
title = 'TracebackException or StackSummary.extract with capture_locals=True fail to catch exceptions raised by repr() on value of frame local variable in FrameSummary.__init__.'
updated_at = <Date 2021-11-25.14:52:05.471>
user = 'https://github.com/moi90'

bugs.python.org fields:

activity = <Date 2021-11-25.14:52:05.471>
actor = 'iritkatriel'
assignee = 'none'
closed = False
closed_date = None
closer = None
components = []
creation = <Date 2021-03-29.09:36:49.757>
creator = 'moi90'
dependencies = []
files = []
hgrepos = []
issue_num = 43656
keywords = ['patch']
message_count = 43.0
messages = ['389679', '389680', '389794', '389815', '389819', '389840', '389842', '393199', '393298', '393519', '404319', '404330', '404392', '404445', '404452', '404456', '404457', '404464', '404468', '404514', '404567', '404603', '404736', '404950', '404988', '404989', '405028', '405033', '405049', '405214', '405238', '405893', '405918', '405935', '405994', '406008', '406012', '406042', '406107', '406997', '406998', '406999', '407001']
nosy_count = 6.0
nosy_names = ['rbcollins', 'serhiy.storchaka', 'iritkatriel', 'moi90', 'andrei.avk', 'jbw']
pr_nums = ['25062', '29299']
priority = 'normal'
resolution = 'not a bug'
stage = 'patch review'
status = 'open'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue43656'
versions = ['Python 3.8', 'Python 3.9', 'Python 3.10']

[moi90]

With capture_locals=True, StackSummary.format prints the local variables for every frame:

cpython/Lib/traceback.py

Line 440 in 4827483

if frame.locals:

This will fail, however, if string conversion fails.

StackSummary.format should be robust towards such possibilities.

An easy fix would be a utility function:

def try_str(x):
  try:
    return str(x)
  except:
    return "<some sensible hint>"

[moi90]

I have to correct myself: The conversion to string already happens during construction, in FrameSummary.__init__:

cpython/Lib/traceback.py

Line 273 in 4827483

self.locals = {k: repr(v) for k, v in locals.items()} if locals else None

The issue remains as severe and the fix remains as easy.

[serhiy-storchaka]

Why str() is called at all? Would not be better to use repr()?

[moi90]

Yes, it is repr in FrameSummary.__init__.

[iritkatriel]

See bpo-39228.

[moi90]

I didn't know repr is supposed to always succeed. Does the documentation mention this?

[moi90]

I can't find any mention of this in the documentation:

https://docs.python.org/3/reference/datamodel.html#object.\_\_repr__

https://docs.python.org/3/library/functions.html#repr

I think this should be mentioned somewhere.

[iritkatriel]

I think it's the other way around - the documentation usually states what exceptions a function can raise as part of its published API, not which exceptions it should not raise.

[akulakov]

@martin it's more that logic in a function may raise exception and it will be handled somewhere upstream, but a repr should just give you a simple representation of an object, it would make no sense for upstream logic to be "object repr failed with such exception, instead we should do this now". It's hard to imagine when this would make sense.

[moi90]

Thanks for the explanations. I think this issue can be closed then.

[jbw]

I would like to request that this bug be repopened and fixed.

I've changed (or at least tried to change, I'm not sure if it will let me) the title of the bug to point out that the failure happens in FrameSummary.__init__. It does not happen in StackSummary.format.

This problem makes the capture_locals=True feature of TracebackException and StackSummary and the "locals" parameter of FrameSummary unreliable. If any one of the local variables in any frame on the stack is in an inconsistent state such that repr will raise an exception, the processing of the traceback fails. This kind of inconsistent state is of course likely to happen during debugging, which is precisely when you would want the capture_locals feature to actually work so you can see what is going wrong.

Just one example of an exception traceback being created with an unsafe local variable value in one of the stack frames is in the following line:

  from _pydecimal import Decimal as D; D(None)

The _pydecimal.Decimal.__new__ method raises an exception when it sees a value it doesn't know how to convert to Decimal. When it does this, the new object it was creating is left in an inconsistent state missing the _sign attribute. When you try to inspect the resulting exception traceback with traceback.TracebackException(..., capture_locals=True), this raises an exception.

While I was tracking this bug down, I discovered that the traceback.TracebackException.__init__ method has the same problem: it initializes the _str attribute used by the __str__ method quite late and when it raised an exception before this point, the incompletely initialized TracebackException object caused repr to fail. There's at least one more class in traceback.py that also has this problem, but I don't remember which one it is.

The cascade of exceptions causing exceptions causing exceptions makes the capture_locals feature difficult to use and debug.

Here is a short snippet that reproduces the bug on Python 3.9.7:

import traceback
class TriggerTracebackBug:
    def __init__(self):
        raise RuntimeError("can't build a TriggerTracebackBug object for some reason")
        self._repr = 'if we reached this line, this object would have a repr result'
    def __repr__(self):
        return self._repr
try:
    TriggerTracebackBug()
except Exception as e:
    # this method call fails because there is a stack frame with a local
    # variable (self) such that repr fails on that variable's value:
    traceback.TracebackException.from_exception(e, capture_locals=True)

It's clear that it is too much to expect every class to implement a safe __repr__ method. So it also seems clear to me that traceback.FrameSummary.__init__ is the place to fix it.

My suggested fix is to replace this line in the latest Lib/traceback.py:

        self.locals = {k: repr(v) for k, v in locals.items()} if locals else None

with something like this code (written in a somewhat awkward way because that helped while I was debugging it):

        if locals:
            d = {}
            self.locals = d
            for k, v in locals.items():
                try:
                    d[k] = repr(v)
                except Exception:
                    d[k] = '''<an exception was raised while trying to format this local variable's value>'''
        else:
            self.locals = None

I've tested this code in an older version of Python and it fixed the problem for me.

[moi90]

Irit, I'm unsure about the wording. Something like ":meth:`__repr__` should always succeed, even if errors prevent a full description of the object."? "... even if the object is only partly initialized."?

Andrei, I think you can not simply omit the first argument. For virtually all methods, self is an important value for debugging. It could be omitted for init, but that would break consistency. Also, init could call other methods that help initialize the object and therefore also deal with partly initialized objects.

I really think one important part of the solution to this problem is making people aware, that under some rare conditions, repr might receive a partly initialized object.

I'm also still convinced that the other part of the solution is a way to customize the behavior of StackSummary.extract and friends.
Context: I have written a tool (https://github.com/moi90/experitur) to run batches of (machine learning) experiments (so it is not interactive). In case of an error, the traceback is dumped into a file, together with the respective local variables (using traceback.TracebackException.from_exception(...).format()). I want this to succeed in any case, even if I was too ignorant to implement correct __repr__s in my experiment code (which is mostly on-time-use, so why should I care).

In the end, this whole problem only affects users of capture_locals=True, so we could just change the semantics of this parameter a bit:
False: do nothing, True: use repr (as before), : use this callable to convert the value to a string.
This way, we could avoid adding an additional parameter to many of the methods in traceback AND give users an easy way to customize exception formatting for easy debugging.

[iritkatriel]

Martin, how about something like:

"This is typically used for debugging, so it is important that the representation is information-rich and unambiguous. Furthermore, this function should not raise exceptions because that can make it inappropriate for use in some debugging contexts. "

[akulakov]

Martin: It's true that exceptions raised in other methods called from __init__ would still have this issue, but I feel like ~90% of it would be solved by the proposed fix.

It does introduce an inconsistency but it does so because it reflects the inconsistency of self being NOT initialized in init and self being initialized in all other methods used after init. It makes intuitive sense that you don't get a repr of an object before it exists in a full state.

The doc fix is useful in itself, but I'm not sure it's enough given the issue reported by Joe with new students getting this error. When they get this error, they will not understand why it happens, what's the connection between the error and the argument that needs to be provided to fix it, and whether it's the correct fix or not.

If they *do* understand it, fixing the __repr__ is probably the best solution, and then no fix on our side is required.

My concern with adding a safe repr argument is that code using that argument can be copy pasted unknowingly and then it's not explicit/obvious that it will silence the errors raised due to broken __repr__'s. (including __repr__'s that are broken even outside of __init__ method).

If such an parameter is added, it's much better to make it a new parameter, to avoid backwards incompatible change, which means it can be backported.

[jbw]

1. As background, it is worth remembering that a major motivation for why FrameSummary.__init__ stringifies the local variable values in its parameter locals is to prevent the resulting data structure from keeping those values live. This enables them to be garbage collected.

2. It has been suggested that TracebackException.__init__ or StackSummary.extract could be given a function parameter. This could either be a new parameter or could involve using the existing capture_locals parameter with a non-bool. The purpose of this suggestion is to allow customizing the use of repr to stringify local variable values. If this suggestion is followed, it would be quite useful if this function parameter was given not just the local variable values, but also the name of the local variable and maybe also the stack frame it is in. This would enable filtering out undesired variables. For example, I would usually prefer to filter most of the variables from the __main__ module frame, because they are just noise that makes it hard to see the important details. Some ability to filter would also help with the security issue that is discussed in bpo-23597 that Irit pointed to.

3. I doubt that new students will be setting up calls to TracebackException or modifying sys.excepthook on their own. Although a simple interface is clearly good, it might be more important to have an interface that maximizes the usefulness of the feature.

4. I no longer have the tracebacks my students were getting. You can look at the traceback from the example in msg 404319 for a traceback. While I was debugging this, I got much more complicated tracebacks because two of the classes in traceback.py also throw exceptions during their __init__ method that leave the not-yet-initialized object in a repr-will-raise-an-exception state. Despite having decades of programming experience, it actually took me a long time before I realized that the exceptions I was debugging were junk exceptions due to attempts to call repr on not-yet-initialized objects. I think figuring this out would be extremely challenging for my second-year undergraduate students.

5. If one of the calls to repr in FrameSummary.__init__ raises an exception, this prevents all the other local variable values from being inspected. If it is desired to report local variable values that cause repr to raise an exception, then it would be good to collect all such exceptions, which requires handling each exception and then continuing to traverse the traceback stack. Because of point 1 above, it might often be best to turn each such exception into a string. To avoid infinite loops in the debugging/logging tools, it would often be best not to attempt to traverse the traceback stack of each of these extra exceptions. If this argument is a good argument, this seems to mean that my most recent proposed fix is a good balance.

6. It is not clear if there is a reliable way to detect whether repr raises an exception due to an object's initialization having been interrupted, but all of the failures that I observed of this sort were for the variable name “self”. In one case, the repr failure was for a normal local variable of an __new__ method which was not a parameter of this method; the variable was named “self” by convention, but this convention would be difficult to automatically verify. In the two other cases, the repr failure was for a parameter named “self” which was the first parameter of an __init__ method. So it would help to give special treatment to the first parameter of __init__ methods, but this would not solve the problem for __new__ methods.

7. In some cases, it might be a bit complicated to ensure the object is always in a safe state for repr-ing during initialization. This is because there may be many attributes that need to be modified and this is not generally done atomically. One attribute could be designated to indicate that initialization is done, so that repr will be extra careful if that attribute does not have an expected value. Given that this is only (so far) an issue for debuggers and traceback inspection tools, it is not clear how to motivate everyone who writes a __new__, __init__, or __repr__ method to do this safely. Documentation can of course help.

[moi90]

Once again a very good summary, thanks Joe!

it would be quite useful if this function parameter was given not just the local variable values, but also the name of the local variable and maybe also the stack frame it is in

So this would be something like format_locals(filename, lineno, name, locals) -> dict[str,str] that could be used inside FrameSummary.init. I like that!

I wouldn't bother with detecting un-repr-able objects in Python itself, but if the above format_locals was implemented, you could easily prepare such a formatter for your students that hides self et al. and/or catch exceptions during repr to your liking.

What is needed to get an OK for such a change?

[moi90]

Could the participants of this issue please have a look at my pull request:

#29299

What do you like, what don't you like? Does it work for your use case?

[akulakov]

Martin: I'm not sure what is the best way to fix this issue, so I hope someone else will look into this.

[moi90]

I see two scenarious discussed here:

Scenario 1 (Offline / Batch-Mode):
A system runs user-supplied jobs that may fail. In the case of an error, the system shouldn't crash but rather store a maximally helpful message and carry on with the next job. Most likely, the relevant information is the situation that lead to the error in the first place, not that repr() has also gone wrong as a result.

This could be a a system to automatically test the homework code of your students. Or, like in my case, a framework that runs experiments. You would very likely want a system that does not crash if a __repr__ is wrongly implemented but prints a readable traceback and carries on with the next job.

Here, format_locals could be used to fall back to a different representation of an object if repr() fails.

This is the use case that my pull request tries to address primarily.

Scenario 2 (Online / Write, Test, Repeat):
A user frequently rewrites and executes their code until the desired outcome appears.
Errors inevitably happen. In this case, a maximally helpful stack trace is needed. Again, the relevant information is the situation that lead to the error in the first place, not that repr() has also gone wrong as a result.

Yes, it would be hard for unexperienced users to come up with StackSummary.extract(format_locals=...) by themselves.
But, the correct way would be to use a debugger anyway, not some hand-written code to print exceptions on the fly.

However, if your students receive a template file where they fill in missing function bodies etc, there might already be a substantial amount of code which they don't understand at first, nor do they need to care. Therefore, a piece of code that formats exceptions nicely would hurt here.

I think the most useful change for Scenario 2 (if, for some reason, a proper debugger is not an option) could be to add a command line option (e.g. -X capture_locals) so that a complete stack trace is printed if an exception bubbles up to interpreter level. (Here, it wouldn't hurt to handle exceptions in repr() because the interpreter already stopped exection anyway.)
This option would be very easy to teach to inexperienced users.

My pull request would provide preparation for this more extensive change.

[akulakov]

Martin:

I have a couple of concerns:

• Generally (AFAIK) Python is very conservative about silencing arbitrary exceptions. There are a few functions with args like ignore_errors, but those are for errors in the logic of respective functions. I don't recall examples where any error would be silenced via an argument, but if there are such cases, it would be interesting to look into how the design decision was made.

In this case of course arbitrary exceptions coming any objects' __repr__ may be silenced.

There is a clear and very explicit way to catch exceptions via try/except and as a dev, I would really want to be able to look at a module, look at all try/except clauses and be confident that exceptions are not silenced elsewhere.

• You are targeting this fix to production use, but realistically, if merged, it will be used both in testing and production. Which means, by not seeing these exceptions in testing, you will have a higher chance of deploying them to production where they can surface in other circumstances.

IOW, as a dev I might prefer to see these errors early and often, rather than have a mechanism that ends up silencing errors more broadly than intended.

I'm not saying this fix should be rejected, but that there's a tricky balance here -- and I don't feel confident enough about this solution to approve the PR if I reviewed it.

[moi90]

Just to avoid misunderstandings: My pull request is not at all about silencing exceptions. It is about customizing the output of the traceback module. (Just like the introduction of capture_locals previously: bpo-22936)

(-X capture_locals, on the other hand, is a hypothetical idea that might help with debugging, but we don't have to discuss this now.)

My main argument for the proposed change is that traceback is useless in certain situations, because capture_locals is not "robust" (it *might* raise exceptions; I could handle these in the calling code but that would again hide the stack trace that I was about to investigate) and might dump sensitive data like passwords into logfiles (msg237320, msg237323).

Nothing is taken away or hidden, but flexibility is added.

(The only other option to resolve these issues would be to re-implement much of the current functionality of traceback in a third-party module, which would lead to maintainability problems and fragmentation.)

[moi90]

I improved the example in traceback.rst to illustrate how format_locals works.

[jbw]

Some quick comments on Martin's pull request.

1. The changes are sufficient to let the user make things work the way it is requested that they work and anyone who does not start using the new format_locals parameter will get the old behavior.

2. A side comment: I just noticed that the docstring for FrameSummary.__init__ does not document the filename, lineno, and name parameters. Perhaps this could be fixed at the same time?

3. The new parameter format_locals is slightly confusingly named, because it does not format a single string from all the locals but instead gives a dictionary of strings with one string for each local.

4. I personally think it would be better to include something like the example value for format_locals as an extra attribute of the traceback module so everybody doesn't have to write the same function. That said, the documented example is sufficient.

5. It is not clear to me why this stringify-ing of the locals can't be done in StackSummary._extract_from_extended_frame_gen. It passes the dictionary of locals and also the function to transform it to FrameSummary. It would make more sense to transform it first. I suppose there might be some user somewhere who is directly calling FrameSummary so the interface needs to stay.

6. I fantasize that the new format_locals parameter would have access to the frame object. In order for this to happen, the frame object would have to be passed to FrameSummary instead of the 3 values (locals, name, filename) that are extracted from it. I think the current interface of FrameSummary was designed to interoperate with very old versions of Python. Oh well.

7. If anyone wanted to ever refactor FrameSummary (e.g., to enable my fantasy in point Add Pycharm's .idea directory to gitignore #6 just above), it becomes harder after this. This change has the effect of exposing details of the interface of FrameSummary to users of StackSummary.extract and TracebackException. The four parameters that the new parameter format_locals takes are most of the parameters of FrameSummary.

[moi90]

Thanks Joe!

1. The changes are sufficient to let the user make things work the way it is requested that they work and anyone who does not start using the new format_locals parameter will get the old behavior.

That was my goal :)

2. A side comment: I just noticed that the docstring for FrameSummary.__init__ does not document the filename, lineno, and name parameters. Perhaps this could be fixed at the same time?

I agree and already updated the pull request.

3. The new parameter format_locals is slightly confusingly named, because it does not format a single string from all the locals but instead gives a dictionary of strings with one string for each local.

I think format_locals is OK here (local*s*: plural), but I'm open to better suggestions.

4. I personally think it would be better to include something like the example value for format_locals as an extra attribute of the traceback module so everybody doesn't have to write the same function. That said, the documented example is sufficient.

I sort of agree, but I also don't know what such a general-purpose function would entail. Suggestions?

5. It is not clear to me why this stringify-ing of the locals can't be done in StackSummary._extract_from_extended_frame_gen. It passes the dictionary of locals and also the function to transform it to FrameSummary. It would make more sense to transform it first. I suppose there might be some user somewhere who is directly calling FrameSummary so the interface needs to stay.

I agree! In principle, FrameSummary has been little more than a container without (much) logic of its own.
Memory efficiency requires that that FrameSummary does not hold references to the original locals, thats why repr() was used.
I think as well that it would have been better to keep FrameSummary as a mere container and do the conversion of the locals elsewhere.
But at this point, this shouldn't be changed anymore.

6. I fantasize that the new format_locals parameter would have access to the frame object. In order for this to happen, the frame object would have to be passed to FrameSummary instead of the 3 values (locals, name, filename) that are extracted from it. I think the current interface of FrameSummary was designed to interoperate with very old versions of Python. Oh well.

Do you have a use case for that? I have no clue what you would do with the frame object at this point.

7. If anyone wanted to ever refactor FrameSummary (e.g., to enable my fantasy in point Add Pycharm's .idea directory to gitignore #6 just above), it becomes harder after this. This change has the effect of exposing details of the interface of FrameSummary to users of StackSummary.extract and TracebackException. The four parameters that the new parameter format_locals takes are most of the parameters of FrameSummary.

Previously, I totally misread your request to "not just the local variable values, but also the name of the local variable and maybe also the stack frame it is in". This is why I included filename, lineno and name as parameters to format_locals which now seems totally useless to me and I'll remove them (if nobody objects).

[moi90]

Irit, would you be able to take a look at the patch?

---

I found another application scenario for the patch: In Numpy and Pandas, the assert_* functions in testing have a __tracebackhide__ local that advises pytest to hide the frame. With the patch in place, we could at least not display any locals if __tracebackhide__ is present.

[iritkatriel]

While I do think this module should be more customisable than it is, I think it should be done via support for subclassing rather than injecting functions that get forwarded on as you do here.

There are other issues on bpo related to customising this module, with more convincing use cases than suppressing errors. I think it’s more likely that a patch will be accepted which solves the general problem of this module being inflexible (while being backwards compatible).

The patch you propose here solves a small part of the problem while making the api clunkier and it commits us to supporting this new parameter when we try to solve the general problem.

[moi90]

Thanks for your definitive answer, this is what I was waiting for.

I understand and I totally agree that subclassing is the way to go to make traceback more flexible.

Would you mind linking the other issues concerning the general improvement of traceback?

[iritkatriel]

Try the search function on the tracker (that's what I would need to do to find what to link).

[iritkatriel]

While I don't think it's a good idea to teach python learners that swallowing exceptions is a good idea, and it's easy enough to rerun the student's homework with capture_locals=False, the situation may be different in large prod jobs that fail intermittently. In a case like that the problem may not be easily repeatable, and the traceback module should prioritise the original exception over any rendering errors. While it is a little weird to use capture_locals in a production job, people may want to do it.

Given that we have the _safe_string utility which is used in a few other places for this purpose (for either repr or str), I don't think there is much harm applying it to the locals as well.

@moi90 - Would you like to submit a path with this change, plus some unit tests and doc update? This is a new feature rather than a bugfix so we should not backport it.

--- a/Lib/traceback.py
+++ b/Lib/traceback.py
@@ -279,7 +279,8 @@ def __init__(self, filename, lineno, name, *, lookup_line=True,
         self._line = line
         if lookup_line:
             self.line
-        self.locals = {k: repr(v) for k, v in locals.items()} if locals else None
+        self.locals = {k: _safe_string(v, 'local', func=repr)
+                          for k, v in locals.items()} if locals else None
         self.end_lineno = end_lineno
         self.colno = colno
         self.end_colno = end_colno

[moi90]

Like this? :)

[iritkatriel]

Yes, I suggested a rewording of the news file. You can click the button to accept it if you're happy with it.