Opened 4 years ago

Closed 14 months ago

#16245 closed Cleanup/optimization (fixed)

send_robust should include traceback in response when Exception occurs

Reported by: jsdalton Owned by: unaizalakain
Component: Core (Other) Version: master
Severity: Normal Keywords: signals
Cc: jsdalton, unai@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I have cron job that runs a task which sends a signal via send_robust. I ran into a wall recently in trying to configure better logging for this job. The basic problem is that I can't access the traceback for any exception that occurred in a receiver, which makes debugging in my situation something of a nightmare.

As it stands now, if an exception occurs in a receiver when a send_robust signal is sent, a tuple is added to the responses list which contains the receiver function and the exception.

I would like to propose that we include the traceback as a third item in the tuple. With the exception and traceback, it is possible to construct the (type, value, traceback) returned by sys.exc_info() (and used by the Python logging module), so that exceptions in receiver functions can be more effectively debugged.

A patch will require no more than a line or two in production and test code and probably a minor documentation tweak.

I will submit a patch for this shortly, unless anybody presents any objections or concerns.

Attachments (3)

add_traceback_to_send_robust.diff (3.9 KB) - added by jsdalton 4 years ago.
add_traceback_to_send_robust.v2.diff (4.5 KB) - added by jsdalton 4 years ago.
add_traceback_to_send_robust.v3.diff (4.7 KB) - added by jsdalton 4 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 follow-ups: Changed 4 years ago by jsdalton

  • Cc jsdalton added
  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Patch for this ticket attached.

The most significant aspect of this patch is the change in the documentation. I had to reword the signals documentation a bit to make clear that a three tuple with the traceback is appended to the list returned by send_robust() instead of the usual (receiver, response) tuple pair.

I spent a bit of time considering whether adding the traceback to the tuple was the cleanest approach. It muddies the API a bit, since previously a tuple pair was always appended no matter what. It has the advantage of not breaking any existing code since the first two items in the tuple are exactly the same as they were before the change.

In researching this patch I also learned that all this is unnecessary in Python 3.0. Per PEP 3134 (http://www.python.org/dev/peps/pep-3134/), Exception objects in Python 3.0 have a traceback attribute which hold the traceback object for the call stack at the point where the exception occurred.

Changed 4 years ago by jsdalton

comment:2 in reply to: ↑ 1 ; follow-up: Changed 4 years ago by aaugustin

  • milestone 1.4 deleted
  • Triage Stage changed from Unreviewed to Design decision needed

Replying to jsdalton:

I spent a bit of time considering whether adding the traceback to the tuple was the cleanest approach. It muddies the API a bit, since previously a tuple pair was always appended no matter what. It has the advantage of not breaking any existing code since the first two items in the tuple are exactly the same as they were before the change.

I don't understand; what about:

for receiver, err in my_signal.send_robust():
    ...

comment:3 in reply to: ↑ 2 Changed 4 years ago by anonymous

Replying to aaugustin:

I don't understand; what about:

for receiver, err in my_signal.send_robust():
    ...

Apologies if I was not clear in my ticket description. The traceback containing the call stack at the point when the exception occurred is not available in the exception instance returned (or the receiver function).

Imagine a situation for example where you had five receiver functions being called for a given signal execution. Let's pretend that an unhandled exception was thrown in three of those. You would like to log not only what error occurred in each receiver, but also the call stack leading to the error so it can be effectively debugged. There is no way to obtain the call stack from the error message returned by send_robust(), and even something like sys.last_traceback() will only give you the previous traceback, not all three.

Does that make sense?

Of course, If I'm overlooking some completely obvious way to get at the traceback, please do let me know..

comment:4 follow-up: Changed 4 years ago by aaugustin

I understand the problem you're trying to solve.

What I don't understand is why you say your solution is backwards compatible. I think the code I've shown is a common pattern, and it would break after your patch, wouldn't it?

comment:5 in reply to: ↑ 4 Changed 4 years ago by jsdalton

Replying to aaugustin:

I understand the problem you're trying to solve.

What I don't understand is why you say your solution is backwards compatible. I think the code I've shown is a common pattern, and it would break after your patch, wouldn't it?

Ah, thanks. I misunderstood your point of concern. You are indeed correct. I overlooked this usage pattern for a list of tuples, which would break with this change.

I considered two other API alternatives, though I was not at the time particularly satisfied with either:

  • Rather than returning the error instance as the second argument to the tuple pair, return the three tuple (type, value, traceback) -- i.e. the same three tuple returned by sys.exc_info(), where type is the Exception class and value is the exception instance. This of course also breaks current implementations.
  • Add a private (or otherwise) variable to the exception instance, e.g. __traceback__, a la Python 3. I mentioned this in my first comment above. This solution does not break current implementations. It solves the problem. It's also in keeping with a design decision that the larger Python community agreed is a "good" one, i.e. to attach call stack information about the error to the exception instance.

I'm not sure which of these alternatives I prefer. Since my original solution breaks current implementations, I'm probably least satisfied with it. The first alternative I propose (adding the three tuple in place of the error instance) is logical and also useful. For example, if you're passing error information on to a logging object (that's my particular use case), you can pass that second item in the tuple directly to the exc_infokwarg and the logging class will handle communicating all the necessary info for you. I think this is the "best" solution if it was agreed that it was acceptable to break current implementations with this change.

The third solution (adding a traceback attribute) has the advantage of being highly unlikely to break existing implementations. It's possible that it still could break them, e.g. if __traceback__ was the chosen attribute and someone was already assigning that during error handing, and this overwrote it. It's of course highly unlikely that the new traceback would be different than the one assigned, but the possibility exists. It also feels a bit weird to me to just add an arbitrary attribute to an object we know nothing about, except that it's probably an exception object. There may be stronger, more logical reasons than just "it feels weird". Anyhow, strong advantage of this is it doesn't break current implementation, weakness is it's the messiest and probably most confusing and "magical" seeming.

Not sure if this is the appropriate place to continue this discussion, and perhaps it should be moved to the developers list. But if anyone has feedback on the above I'd be interested in hearing it.

comment:6 Changed 4 years ago by jsdalton

Another idea, after giving this a bit more thought.

Perhaps my original solution could be made backwards compatible by adding a kwarg to send_robust() as follows:

def send_robust(append_traceback=False):
   ...

If append_traceback is set to true, then the traceback is included as a third item in the appended tuple. If not it returns a tuple pair as it does at present.

Existing implementations would work as is, but if for some reason you required the traceback for debugging or logging purposes, you could set the flag and be sure you were handling the returned list properly.

This seems like the least intrusive change we could make assuming it's agreed that my ticket is worth accepting.

comment:7 Changed 4 years ago by jsdalton

I have updated the patch to reflect my last comment. Adding the traceback as a third item the tuple response is now an option via append_traceback, the default for which is False.

Existing implementation of send_robust() will now work as is. To make use of my proposed feature, you would need to set append_traceback to True when calling send_robust().

If anyone has any objections or concerns I'd like to hear them. Accessing the traceback at the point an exception occurs in a receiver function is currently not possible. This makes it difficult if not impossible to debug if something goes wrong in one of the receiver functions.

Changed 4 years ago by jsdalton

comment:8 Changed 4 years ago by jsdalton

Changed the API in response to good feedback from jdunck on the developers list here (https://groups.google.com/d/topic/django-developers/iyI2avkuIpM/discussion). Basically the changes from the previous version are:

  • Change name of kwarg from "append_traceback" to "exc_info"
  • Instead of appending the traceback as the third item in the response, instead just return the tuple from sys.exc_info(), which contains the exception and traceback.

Updated patch (including updated tests and documentation) attached.

Changed 4 years ago by jsdalton

comment:9 Changed 4 years ago by jdunck

+1 on this patch.

comment:10 Changed 4 years ago by jdunck

  • Owner changed from nobody to jdunck
  • Status changed from new to assigned

comment:11 Changed 4 years ago by jdunck

  • Owner changed from jdunck to nobody
  • Status changed from assigned to new

Hmm, I can't own since I'm not a core committer.

comment:12 Changed 4 years ago by carljm

  • Triage Stage changed from Design decision needed to Accepted

comment:13 in reply to: ↑ 1 Changed 3 years ago by development@…

Replying to jsdalton:

In researching this patch I also learned that all this is unnecessary in Python 3.0. Per PEP 3134 (http://www.python.org/dev/peps/pep-3134/), Exception objects in Python 3.0 have a traceback attribute which hold the traceback object for the call stack at the point where the exception occurred.

If traceback is an attribute of the exception in python 3.0, then why don't we emulate that here? Add the traceback as an attribute of the returned exception.

Either way, I hope this is able to get into 1.4. Its definitely needed.

comment:14 Changed 21 months ago by unaizalakain

  • Cc unai@… added

+1 on the last proposed solution (and second solution given in comment:5): if not in python3, just emulate it.

comment:15 Changed 20 months ago by unaizalakain

  • Owner changed from nobody to unaizalakain
  • Status changed from new to assigned

comment:16 Changed 20 months ago by unaizalakain

  • Has patch unset

Mailing list discussion: https://groups.google.com/d/msg/django-developers/nyF8wb-GUaI/m14hBDZ49AwJ
I'm submitting a patch with the __traceback__ way.

comment:18 Changed 20 months ago by unaizalakain

  • Has patch set

comment:19 Changed 14 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In ebb0279f4a7a7155c44c09506bbe5b1f9acc83a2:

Fixed #16245 -- Included traceback in send_robust()'s response

Exceptions from the (receiver, exception) tuples returned by
send_robust() now have always their traceback attached as their
traceback argument.

Note: See TracTickets for help on using tickets.
Back to Top