Opened 13 years ago
Last modified 9 years ago
#16674 new Bug
Django's WSGI Handler should report exceptions to the start_response() callback
Reported by: | James Henstridge | Owned by: | nobody |
---|---|---|---|
Component: | HTTP handling | Version: | dev |
Severity: | Normal | Keywords: | |
Cc: | James Henstridge, Graham Dumpleton | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
The WSGI specification allows WSGI applications to pass an exception context to their start_response() callback if the response is being provided by an error handler.
This is quite useful for WSGI middleware that collects data about errors: they can log the problem and then pass on the error page generated by the application.
Unfortunately, Django's WSGI handler never seems to call start_response() with the third argument, preventing this sort of middleware from seeing errors in Django applications.
I think a solution to this would be something like:
- override handle_uncaught_exception() in WSGIHandler and make it stash exc_info somewhere (on the response object would be hacky but maybe workable).
- after call invokes get_response(), check for a captured exc_info value.
- if exc_info was captured, pass it to start_response()
Attachments (2)
Change History (16)
comment:1 by , 13 years ago
Cc: | added |
---|
comment:2 by , 13 years ago
comment:3 by , 13 years ago
Cc: | added |
---|
comment:4 by , 13 years ago
By my reading of the WSGI spec, that is not the case. If the app does something like the following, then the container will render the custom error page:
def simple_app(environ, start_response): try: raise RuntimeError() except RuntimeError: start_response('500 Internal Error', [('Content-type', 'text/plain')], sys.exc_info()) return ['Custom error message']
The spec only says that the container may re-raise the exception if the headers have already been sent, which is clearly not the case if the app only calls start_response() once, as Django's WSGIHandler does. This seems pretty clear from both the wording in the PEP and the wsgiref reference implementation.
by , 13 years ago
Attachment: | wsgi-expose-exc-info.patch added |
---|
comment:5 by , 13 years ago
Has patch: | set |
---|
I've attached a patch that offers a sample implementation of what I'm after. With this patch, I was able to capture error reports at the WSGI level with both the DEBUG=True error page and with custom handler500 error pages.
I'm not particularly happy with the way I'm capturing the exception context in the handle_uncaught_exception() method. Perhaps temporarily storing the context on the request object would be a cleaner way of passing it up to call() compared to using thread local storage. WSGIHandler is using its own request class after all.
by , 13 years ago
Attachment: | wsgi-expose-exc-info-v2.patch added |
---|
A modified version of the patch that doesn't use thread local storage
comment:6 by , 13 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
The reference is here: http://www.python.org/dev/peps/pep-3333/#error-handling
If no output has been written when an exception occurs, the call to start_response will return normally, and the application will return an error body to be sent to the browser.
==> Since Django calls start_response
only once, there hasn't been any output at this point, so it's safe to pass exc_info
when exceptions occur. My understanding is that the server shouldn't trap such exceptions and return a generic error page. I haven't tested if common WSGI implementations are compliant, though.
Some middleware may wish to provide additional exception handling services, or intercept and replace application error messages. In such cases, middleware may choose to not re-raise the exc_info supplied to start_response, but instead raise a middleware-specific exception, or simply return without an exception after storing the supplied arguments.
==> This is exactly what the OP is trying to achieve.
These techniques will work as long as application authors:
- Always provide exc_info when beginning an error response
- Never trap errors raised by start_response when exc_info is being provided
==> I think that's what Django should do.
I agree with Graham that the difficult problem is to obtain the exception, especially if it's been caught and handled by the application.
However, I think it would be an interesting step towards WSGI compliance to pass to start_response
the exc_info
of exceptions handled by handle_uncaught_exception
.
Marking as "DDN" until we decide how much we want to fix in the context of this ticket.
comment:7 by , 13 years ago
For what it is worth, capturing the exception info from handle_uncaught_exception() covers mode of what I need, since that is where failure in my own code usually get caught. Even if it doesn't give total coverage of all errors that Django intercepts, it would still be a useful improvement.
In case you're interested, the WSGI middleware I am trying to get working is python-oops-wsgi:
comment:8 by , 13 years ago
Triage Stage: | Design decision needed → Accepted |
---|
I'd like some verification from Graham that exposing the exc_info won't squash Django's own error page, but if it won't this sounds like a good idea to me.
comment:9 by , 13 years ago
For what it is worth, I haven't noticed any problems using the patch I attached to the bug report (both for normal Django error pages and the developer debug error page).
The wsgiref module ignores the exc_info argument provided that the response headers haven't been sent (not a problem for Django, since it only calls start_response once).
The same seems to be true for mod_wsgi:
http://code.google.com/p/modwsgi/source/browse/mod_wsgi/mod_wsgi.c#2600
And similar for Paste:
https://bitbucket.org/ianb/paste/src/852439f67241/paste/httpserver.py#cl-153
https://bitbucket.org/ianb/paste/src/852439f67241/paste/modpython.py#cl-176
And Twisted:
http://twistedmatrix.com/trac/browser/trunk/twisted/web/wsgi.py#L256
comment:10 by , 13 years ago
Is there anything else I can do to help get things moving on this ticket? From my own tests and the source code references in my previous comment, I think Jacob's concerns about hiding the Django error pages are addressed. So is there anything else that needs to happen?
comment:11 by , 12 years ago
Triage Stage: | Accepted → Unreviewed |
---|---|
Type: | Uncategorized → Bug |
I pushed a github pull request with the attached patch (and added tests). It can be found here: https://github.com/django/django/pull/133
comment:12 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
"Unreviewed" relates to the ticket, not the patch.
comment:14 by , 9 years ago
Component: | Core (Other) → HTTP handling |
---|
Doing that would then cause the exception to propagate all the way up and you would loose the default Django error page and at worst get a generic web server/WSGI server error page. That one place isn't sufficient anyway if your intent is to catch details of any errors. This is because that function is only called when there is no error response middleware to render a custom error page. You would therefore miss those errors when a error response middleware generated an error page. Finally, that function also possibly isn't called when there is an error with view handler lookup by the URL resolver.
The solution therefore isn't to try and propagate stuff up to start_response(), but for Django to provide a callback/event mechanism for being notified of exceptions in the various contexts they can occur prior to being consumed within the Django framework and an error page of some sort generated. That way you could register a callback to be notified of such things.
Agree though from own experience in having to do it, that it is currently a pain to monkey match all the places required to capture errors and notification mechanism would be nice.