Opened 4 years ago
Closed 4 years ago
#31672 closed Cleanup/optimization (fixed)
debug error view shows no traceback if exc.__traceback__ is None for innermost exception
Reported by: | Chris Jerdonek | Owned by: | Hasan Ramezani |
---|---|---|---|
Component: | Error reporting | Version: | dev |
Severity: | Normal | Keywords: | technical_500_response, traceback, chain |
Cc: | Triage Stage: | Ready for checkin | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
Consider this test view which raises an exception:
class TestView(View): def get(self, request, *args, **kwargs): try: raise RuntimeError('my error') except Exception as exc: # Uncommenting these two lines causes the debug # server error view not to show **any** portion # of the traceback. # new_exc = RuntimeError('my context') # exc.__context__ = new_exc raise
If the indicated lines are uncommented, then the debug error view shows no traceback info at all.
This is because django.views.debug.get_traceback_frames()
starts at the innermost exception in the chain, and then stops adding frames when it encounters the first exception with None
-valued exc_value.__traceback__
:
https://github.com/django/django/blob/38a21f2d9ed4f556af934498ec6a242f6a20418a/django/views/debug.py#L391
exc_value = exceptions.pop() tb = self.tb if not exceptions else exc_value.__traceback__ while tb is not None: # Get frame. ... frames.append({ ... }) # If the traceback for current exception is consumed, try the # other exception. if not tb.tb_next and exceptions: exc_value = exceptions.pop() tb = exc_value.__traceback__ else: tb = tb.tb_next return frames
It would probably be better and simpler if instead the while loop were structured to iterate over every exception in the chain no matter what, and include traceback info only for those exceptions with non-None
__traceback__
. In other words, something like the following:
while exceptions: exc_value = exceptions.pop() # Returns placeholder frame with no `tb` info if # `exc_value.__traceback__ is None`. exc_frames = self.get_traceback_frames(exc_value) frames.extend(exc_frames)
This way, the debug error view would always show the complete exception chain, even if some exceptions in the chain have a None
-valued traceback attribute.
Change History (23)
comment:2 by , 4 years ago
Triage Stage: | Unreviewed → Accepted |
---|---|
Type: | Uncategorized → Cleanup/optimization |
Version: | 3.0 → master |
Thanks for the report. Would you like to prepare a patch?
comment:3 by , 4 years ago
Sure, I'll see if I can find time to put something together for this and the others.
By the way, have there been any issues with people not receiving all email notifications from code.djangoproject.com
? It seems like I don't get some of them (not even to my spam folder).
comment:5 by , 4 years ago
Has patch: | set |
---|
comment:6 by , 4 years ago
Patch needs improvement: | set |
---|
comment:7 by , 4 years ago
Patch needs improvement: | unset |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:9 by , 4 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:10 by , 4 years ago
Thanks for fixing the issue!
I looked at the PR after this was merged and made a comment: https://github.com/django/django/pull/13513#issuecomment-707355649
By the way, I didn't get another email about this ticket until after the PR was merged and approved. I think it might be a good addition to your process in general if you could CC the reporter on any PR or otherwise notify them (maybe this could be done automatically) to ensure they also have a chance to see it before it's merged. I know this isn't the first time I didn't have a chance to see a fix for an issue I reported until after it was merged. In many cases, the extra pair of eyes can help because the original reporter may have insights the PR author doesn't have since they spent the time finding and diagnosing it.
comment:11 by , 4 years ago
Chris, thanks for comments. Reporters should get notifications about all updates. I'm not sure why you didn't get them. 🤔
In many cases, the extra pair of eyes can help because the original reporter may have insights the PR author doesn't have since they spent the time finding and diagnosing it.
Totally agreed, I will ping you directly in PRs in the future, thanks.
comment:12 by , 4 years ago
Thanks, Felix!
Regarding the notifications, I have all the "Notify" settings enabled in my Django tracker preferences. It seems like not all modifications trigger an email, though. For example, adding a comment seems to always trigger an email, but not all status changes seem to trigger one.
comment:13 by , 4 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
Triage Stage: | Ready for checkin → Accepted |
We should also handle exceptions without a traceback (as suggested by Chris), this can be achieved by yielding a mostly empty frame from get_exception_traceback_frames()
when tb is None
. Technical 500 templates need to be adjusted as well.
comment:14 by , 4 years ago
Has patch: | unset |
---|
comment:17 by , 4 years ago
Patch needs improvement: | set |
---|
comment:18 by , 4 years ago
Patch needs improvement: | unset |
---|
comment:19 by , 4 years ago
Patch needs improvement: | set |
---|---|
Status: | new → assigned |
comment:20 by , 4 years ago
Patch needs improvement: | unset |
---|
comment:21 by , 4 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
comment:23 by , 4 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
Here is a related (but different) issue about the traceback shown by the debug error view ("debug error view doesn't respect exc.suppress_context (PEP 415)"): https://code.djangoproject.com/ticket/31674