DebugNodeList breaks exceptions

[Glenn Maynard]

DebugNodeList catches all exceptions, sticks them in a TemplateSyntaxError, and stuffs the original exception in the new exception. I'm not sure why this is done, but it breaks debugging and exception handling.

• It masks the backtrace. Django's ExceptionReporter receives and reports the wrapper exception, never seeing the wrapped exception, so the backtrace starts at DebugNodeList's "raise wrapped" line--the location of the actual error is lost. This makes errors inside templates very hard to debug: the only evidence of the real error is the error string. The same problem will happen with any custom exception reporter.
• It's the wrong exception. If my template calls a filter which raises NameError, it's not a template syntax error; it's a NameError.
• It blocks exceptions which were raised intentionally. If I call a filter which raises IOError, the code calling the template should be able to catch that.

I'm attaching two alternate patches:

#1 (template-dont-wrap-exception.diff): Patch removes the TemplateSyntaxError wrapping; it just attaches the source to all exceptions and re-raises. With this patch, exceptions coming out of template filters, etc. can be caught properly when template debugging is enabled, and are reported correctly by standard exception reporters.

#2 (template-fix-lost-traceback.diff): Includes the original tb when re-raising the exception. This is the correct way to raise exceptions like this, when that's really what you want to do. This fixes backtraces only, which is the most annoying problem. It doesn't fix the problems caused by changing the exception type. I include this because it's straightforwardly committable (as I don't know the original reason for the wrapped exception), but I strongly prefer #1.

(related: ticket #11368)

[Chris Beaven]

Good work, Glenn. Django's wrapping of the real traceback has frustrated me for quite some time - I spent some time today solving it after missing this ticket in a quick search and came up with something similar to the thinking in #11368 (I'll attach the patch here)

Thinking about your comment there ("This isn't the right fix. This would have to be implemented in every custom exception reporter, including third-party ones that don't know about Django exceptions at all"), I wouldn't think that someone running standard exception reporters should be running in DEBUG so I'd be content if it got fixed there.

Django's current code does seem quite a lot of work just to prepend the error message with "Caught an exception while rendering: ", but I think that the old thought process (before reverse errors were allowed to bubble) was that a template should never raise an error to begin with, so it should be a TemplateSyntaxError if it does. Maybe. Now I'm just rambling.

In any case, *something* needs fixing. I'm promoting to a design decision - this needs to be brought up after 1.1 settles down.

[Chris Beaven]

(actually, my patch has mostly the same effect as your second method, but not as consise - I'll leave these as the two options)

[John Firebaugh]

I've applied the second patch to my local copy of Django, and it really makes debugging template errors much easier. Please consider applying one of these patches or a similar change for 1.2.

[HM]

Oh yes please, would need a lot less logging for debug-purposes if this was fixed.

[physicsnick]

Second patch works perfectly, solves all problems, thanks. I don't really see the point of the first patch though; it seems like a really bad idea to have your templates able to throw IOErrors. I'm not sure how you expect to recover when a template filter throws an exception anyway.

[Glenn Maynard]

If you're going to claim that something's a "really bad idea", please explain why; I can't think of any reason, even a heavily contrived one. Templates can already throw IOError; DebugNodeList is only used when template debugging is enabled. Whether I can usefully handle exceptions thrown by templates is up to me.

The question is why it was doing this in the first place--hopefully, someone remembers. It's not needed for the "source" attribute; views.debug.get_traceback_html doesn't care about the exception type in order to use it. It's perfectly reasonable to augment exceptions in-place with new attributes directly, except that it should use a less generic attribute name (eg. "template_source").

[physicsnick]

It seems like a bad idea because you seem to have file-handling logic happening during rendering of the template in custom template tags and filters. Shouldn't you be reading this data before trying to render? Any IOError thrown in the rendering automatically aborts the rendering of the template; even if you catch it and do something useful you have to start over the renderer. So why not prepare it beforehand?

As someone mentioned earlier, the reason the template renderer wraps many exceptions is because originally template designers were never supposed to be able to cause 500 errors unless the template is malformed. While I agree that this may cause more problems than it solves, I still think that a template rendering failure should always result in a 500. If you could usefully recover from it, I would think you should instead prepare the data and handle errors before trying to render.

[Glenn Maynard]

If a user wants to catch an exception, it's not up to Django to tell him he can't. It doesn't matter if he's implementing a hack; Django should not deliberately misbehave simply because you don't like what the user is doing.

Not to mention that exception reporters can very easily want to know the type of the exception, to use custom error formatting and handling for different types of exceptions. Exception reporters shouldn't have to deal with hacks like wrapped exceptions (e.exc_info[0]) with exception info split between two (or more) exception objects.

What if I'm using the template engine in an interactive program, and I hit ^{C while a template happens to be rendering? An interactive caller will often want to catch KeyboardInterrupt; it should absolutely not have to catch some obscure template exception and poke through wrapped exceptions in order to handle it--the code doing this likely wouldn't even know about templates itself at all.}

[Glenn Maynard]

(That last example is off--KeyboardInterrupt isn't an Exception, for some reason.)

[Gergely Kontra]

+1 for that this ticket is a must-have. Custom template debugging is a hell without this patch. I don't know why it should wait for 1.2 btw...

[Karen Tracey]

#12294 was a dupe. The effect of this behavior seems to be far worse with Python 2.6 than earlier versions. Previously the original exception traceback was included in the debug page exception value ouput, with Python 2.6 it is missing entirely. (It would be interesting to know if this was an intentional change on the Python side, but I don't have time to research that.)

On the milestone, 1.2 is the next release so that is the earliest milestone it could be assigned to. If there was any question before, I'd say the new Python 2.6 behavior makes this clearly a bugfix, not a new feature, so when it does go in it will also go into the 1.1.X branch and be available in the first 1.1.X release made after that.

[Karen Tracey]

I've attached my proposed fix for this issue. It's based on the 2nd alternative patch included in the original description, which simply includes the original traceback when the exception is turned into a TemplateSyntaxError. (The first alternative is not an option after r12586, since the template source will no longer be included in the debug page if the raised exception is not TemplateSyntaxError.)

In addition to including the original traceback in the raised TemplateSyntaxError, this patch removes the TemplateSyntaxError __str__ method that reports the "original traceback". This __str__ method is not getting called under Python 2.6 and higher, where exceptions have unicode methods. It also doesn't work if there is anything odd about the exception -- such as if it contains non-ASCII data in its args. Finally it seems wrong to have it reporting the "original traceback" when that traceback is now included in the reported traceback.

Comments welcome. I could not find any rationale for moving the original traceback from the traceback itself and into the exception's __str__ method, so it seems best to just switch to preserving the original traceback. If there is some good reason to stick with the old approach then for Python 2.6 and higher we need to add a similar __unicode__ override in TemplateSyntaxError, plus these methods should be made more robust when handed exceptions with non-ASCII data. But it seems easier and better overall to just preserve the original traceback in the raised TemplateSyntaxError.

[Glenn Maynard]

I've filed a bug about r12586 (ticket #12995), which was a bad change. #1 remains the correct solution, and #2 is a very poor fallback. I somewhat regret mentioning it.

[Karen Tracey]

(In [12725]) Fixed #11461: Ensured complete traceback is available on the debug page when an exception is encountered during template rendering, even when running on Python 2.6 or higher. Thanks Glenn.

[Karen Tracey]

(In [12727]) [1.1.X] Fixed #11461: Ensured complete traceback is available on the debug page when an exception is encountered during template rendering, even when running on Python 2.6 or higher. Thanks Glenn.

r12725 and r12726 from trunk.

[Karen Tracey]

For now I've gone with just ensuring the traceback is on the debug page, even when running Python 2.6+. That, at least, I feel needs to be in 1.2/1.1.2. I'm less sure of the necessity of changing the type of exception raised away from TemplateSyntaxError. It makes me a bit nervous from a backwards-compatibility standpoint, since I don't know what might be relying on it programmatically. Also, for a human looking at a deubg page, TemplateSyntaxError at the top is a clue that if you scroll down in the info you should find a template error section showing the part of the template associated with the error; if we get rid of the TemplateSyntaxError we remove that clue. Maybe minor but at this point I'm just not comfortable removing the TemplateSyntaxError wrapping entirely.

[Glenn Maynard]

Wrapping exceptions like this is evil. I'm disappointed in this decision, but I don't have time to argue it.

[Jacob]

Milestone 1.2 deleted