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:

  1. override handle_uncaught_exception() in WSGIHandler and make it stash exc_info somewhere (on the response object would be hacky but maybe workable).
  2. after call invokes get_response(), check for a captured exc_info value.
  3. if exc_info was captured, pass it to start_response()

Attachments (2)

wsgi-expose-exc-info.patch (2.0 KB ) - added by James Henstridge 13 years ago.
wsgi-expose-exc-info-v2.patch (2.3 KB ) - added by James Henstridge 13 years ago.
A modified version of the patch that doesn't use thread local storage

Download all attachments as: .zip

Change History (16)

comment:1 by James Henstridge, 13 years ago

Cc: James Henstridge added

comment:2 by Graham Dumpleton, 13 years ago

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.

comment:3 by Graham Dumpleton, 13 years ago

Cc: Graham Dumpleton added

comment:4 by James Henstridge, 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.

Version 0, edited 13 years ago by James Henstridge (next)

by James Henstridge, 13 years ago

Attachment: wsgi-expose-exc-info.patch added

comment:5 by James Henstridge, 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 James Henstridge, 13 years ago

A modified version of the patch that doesn't use thread local storage

comment:6 by Aymeric Augustin, 13 years ago

Triage Stage: UnreviewedDesign 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:

  1. Always provide exc_info when beginning an error response
  2. 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 James Henstridge, 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:

http://pypi.python.org/pypi/oops_wsgi

comment:8 by Jacob, 13 years ago

Triage Stage: Design decision neededAccepted

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 James Henstridge, 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 James Henstridge, 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 tribaal@…, 12 years ago

Triage Stage: AcceptedUnreviewed
Type: UncategorizedBug

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 Claude Paroz, 12 years ago

Triage Stage: UnreviewedAccepted

"Unreviewed" relates to the ticket, not the patch.

comment:13 by Aymeric Augustin, 12 years ago

Patch needs improvement: set

The patch no longer applies.

comment:14 by Tim Graham, 9 years ago

Component: Core (Other)HTTP handling
Note: See TracTickets for help on using tickets.
Back to Top