[Django] #16245: send_robust should include traceback in response when Exception occurs
Django
----------------------------------+------------------------------
Reporter: jsdalton | Owner: nobody
Type: Cleanup/optimization | Status: new
Milestone: 1.4 | Component: Core (Other)
Version: SVN | Severity: Normal
Keywords: signals | Triage Stage: Unreviewed
Has patch: 0 | Easy pickings: 0
UI/UX: 0 |
----------------------------------+------------------------------
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jsdalton | Owner: nobody
Type: | Status: new
Cleanup/optimization | Component: Core (Other)
Milestone: 1.4 | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Triage Stage: | Needs tests: 0
Unreviewed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by jsdalton):
* cc: jsdalton (added)
* needs_docs: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_better_patch: => 0
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:1>
Django
Reporter: jsdalton | Owner: nobody
Cleanup/optimization | Component: Core (Other)
Resolution: | Has patch: 1
decision needed | Easy pickings: 0
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
* stage: Unreviewed => Design decision needed
* milestone: 1.4 =>
Comment:
Replying to [comment:1 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():
...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:2>
Django
Reporter: jsdalton | Owner: nobody
Cleanup/optimization | Component: Core (Other)
Milestone: | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
Replying to [comment:2 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..
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:3>
Django
Reporter: jsdalton | Owner: nobody
Cleanup/optimization | Component: Core (Other)
Milestone: | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:4>
Django
Reporter: jsdalton | Owner: nobody
Cleanup/optimization | Component: Core (Other)
Milestone: | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by jsdalton):
Replying to [comment:4 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_info`kwarg 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:5>
Django
Reporter: jsdalton | Owner: nobody
Cleanup/optimization | Component: Core (Other)
Milestone: | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:6>
Django
Reporter: jsdalton | Owner: nobody
Cleanup/optimization | Component: Core (Other)
Milestone: | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:7>
Django
Reporter: jsdalton | Owner: nobody
Cleanup/optimization | Component: Core (Other)
Milestone: | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:8>
Django
Reporter: jsdalton | Owner: nobody
Cleanup/optimization | Component: Core (Other)
Milestone: | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Comment (by jdunck):
+1 on this patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:9>
Django
Reporter: jsdalton | Owner: jdunck
Type: | Status: assigned
Milestone: | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => jdunck
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:10>
Django
Reporter: jsdalton | Owner: nobody
Type: | Status: new
Milestone: | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Triage Stage: Design | Needs tests: 0
decision needed | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
Changes (by jdunck):
* status: assigned => new
* owner: jdunck => nobody
Comment:
Hmm, I can't own since I'm not a core committer.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:11>
Django
Reporter: jsdalton | Owner: nobody
Cleanup/optimization | Component: Core (Other)
Milestone: | Severity: Normal
Version: SVN | Keywords: signals
Resolution: | Has patch: 1
Needs documentation: 0 | Easy pickings: 0
UI/UX: 0 |
-------------------------------------+-------------------------------------
* stage: Design decision needed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:12>
Django
Type: Cleanup/optimization | Status: new
Keywords: signals | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by development@…):
Replying to [comment:1 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:13>
Django
Reporter: jsdalton | Owner: nobody
Type: Cleanup/optimization | Status: new
Severity: Normal | Resolution:
Keywords: signals | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by unaizalakain):
* cc: unai@… (added)
Comment:
+1 on the last proposed solution (and second solution given in comment:5):
if not in python3, just emulate it.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:14>
Django
Reporter: jsdalton | Owner:
Type: | unaizalakain
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: signals | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by unaizalakain):
* status: new => assigned
* owner: nobody => unaizalakain
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:15>
Django
Reporter: jsdalton | Owner:
Type: | unaizalakain
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: signals | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by unaizalakain):
* has_patch: 1 => 0
Comment:
Mailing list discussion: https://groups.google.com/d/msg/django-developers
/nyF8wb-GUaI/m14hBDZ49AwJ
I'm submitting a patch with the `__traceback__` way.
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:16>
Django
Reporter: jsdalton | Owner:
Type: | unaizalakain
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: signals | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by unaizalakain):
PR: https://github.com/django/django/pull/1831
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:17>
Django
Reporter: jsdalton | Owner:
Type: | unaizalakain
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: signals | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by unaizalakain):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:18>
Django
Reporter: jsdalton | Owner:
Type: | unaizalakain
Component: Core (Other) | Version: master
Keywords: signals | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"ebb0279f4a7a7155c44c09506bbe5b1f9acc83a2"]:
{{{
#!CommitTicketReference repository=""
revision="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.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:19>
Django
Reporter: Jim Dalton | Owner: Unai
Type: | Zalakain
Cleanup/optimization | Status: closed
Component: Core (Other) | Version: master
Severity: Normal | Resolution: fixed
Keywords: signals | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by GitHub <noreply@…>):
In [changeset:"435e4bf38e97255acd97eacadeb8fe312ba97aff" 435e4bf3]:
{{{
#!CommitTicketReference repository=""
revision="435e4bf38e97255acd97eacadeb8fe312ba97aff"
Refs #23919 -- Removed __traceback__ setting needed for Python 2.
Partially reverted refs #25761 and refs #16245.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/16245#comment:20>