Code

Opened 3 years ago

Last modified 15 months ago

#16674 new Bug

Django's WSGI Handler should report exceptions to the start_response() callback

Reported by: jamesh Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: jamesh, grahamd 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 jamesh 3 years ago.
wsgi-expose-exc-info-v2.patch (2.3 KB) - added by jamesh 3 years ago.
A modified version of the patch that doesn't use thread local storage

Download all attachments as: .zip

Change History (15)

comment:1 Changed 3 years ago by jamesh

  • Cc jamesh added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by grahamd

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 Changed 3 years ago by grahamd

  • Cc grahamd added

comment:4 Changed 3 years ago by jamesh

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.

Changed 3 years ago by jamesh

comment:5 Changed 3 years ago by jamesh

  • 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.

Changed 3 years ago by jamesh

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

comment:6 Changed 3 years ago by aaugustin

  • Triage Stage changed from Unreviewed to 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:

  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 Changed 3 years ago by jamesh

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 Changed 3 years ago by jacob

  • Triage Stage changed from Design decision needed to 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 Changed 3 years ago by jamesh

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 Changed 2 years ago by jamesh

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 Changed 23 months ago by tribaal@…

  • Triage Stage changed from Accepted to Unreviewed
  • Type changed from Uncategorized to 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 Changed 23 months ago by claudep

  • Triage Stage changed from Unreviewed to Accepted

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

comment:13 Changed 15 months ago by aaugustin

  • Patch needs improvement set

The patch no longer applies.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.