Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#4019 enhancement closed fixed (fixed)

t.web.wsgi.WSGIResource - WSGI application error handling

Reported by: osuchw Owned by:
Priority: normal Milestone:
Component: web Keywords:
Cc: Jean-Paul Calderone, therve Branch: branches/wsgi-error-handling-4019
branch-diff, diff-cov, branch-cov, buildbot
Author: exarkun

Description

Currently if there is an unhandled error in a WSGI application the WSGIResource will log the traceback but the request does not finish leaving the client spinning its wheels forever.

According to PEP-333 it is the application responsibility to handle all the errors but it would be nice if WSGIResource returned 500 in such a case.

I did a quick check and it looks like web2.WSGIResource, mod_wsgi and cherrypy do return 500, making the twisted.web version the odd man out.

To demonstrate here are runnable examples. The t.web version

from twisted.web import server
from twisted.web.wsgi import WSGIResource
from twisted.python.threadpool import ThreadPool
from twisted.internet import reactor
from twisted.application import service, strports

wsgiThreadPool = ThreadPool()
wsgiThreadPool.start()

reactor.addSystemEventTrigger('after', 'shutdown', wsgiThreadPool.stop)

def application(environ, start_response):
    1/0
    start_response('200 OK', [('Content-type','text/plain')])
    return ['Hello World!']

wsgiAppAsResource = WSGIResource(reactor, wsgiThreadPool, application)

application = service.Application('Twisted.web.wsgi Hello World Example')
server = strports.service('tcp:8080', server.Site(wsgiAppAsResource))
server.setServiceParent(application)

and t.web2 version

from twisted.web2 import server, channel
from twisted.web2.wsgi import WSGIResource
from twisted.application import service, strports

def application(environ, start_response):
    1/0
    start_response('200 OK', [('Content-type','text/plain')])
    return ['Hello World!']

wsgiAppAsResource = WSGIResource(application)

application = service.Application('Twisted.web2.wsgi Hello World Example')
server = strports.service('tcp:8080', channel.HTTPFactory(server.Site(wsgiAppAsResource)))
server.setServiceParent(application)

Attachments (1)

report_wsgiapp_error.diff (2.3 KB) - added by osuchw 8 years ago.
Updated

Download all attachments as: .zip

Change History (12)

comment:1 Changed 8 years ago by Jean-Paul Calderone

Cc: Jean-Paul Calderone added
Owner: changed from jknight to osuchw

Thanks. Adding error handling to properly finish requests in this case would be great. Do you think you can put together a patch?

comment:2 Changed 8 years ago by osuchw

Well, here is an attempt. No tests though. I would not know where to start with them.

Changed 8 years ago by osuchw

Attachment: report_wsgiapp_error.diff added

Updated

comment:3 Changed 8 years ago by osuchw

updated to work with current trunk

comment:4 Changed 8 years ago by Jean-Paul Calderone

It looks like this belongs in review. But! a cursory glance at the patch reveals no unit tests, so how about I make some suggestions there.

All of the existing tests for this code are in twisted/web/test/test_wsgi.py. ApplicationTests is probably a good case to extent. You can test the various error handling being added by defining various application objects which each exhibit one of the error cases. Then use the lowLevelRender function to issue a request and in a callback on the rendering Deferred, assert that the right things happened (ie, exceptions were logged, the server returned a 500 response, whatever is appropriate).

Thanks!

comment:5 Changed 8 years ago by Jean-Paul Calderone

Owner: changed from osuchw to Jean-Paul Calderone
Status: newassigned

comment:6 Changed 8 years ago by Jean-Paul Calderone

Author: exarkun
Branch: branches/wsgi-error-handling-4019

(In [27522]) Branching to 'wsgi-error-handling-4019'

comment:7 Changed 8 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted
Status: assignednew

comment:8 Changed 8 years ago by therve

Cc: therve added
Keywords: review removed
Owner: set to Jean-Paul Calderone

Looks good. As an additional feature it would be nice if an error page was displayed, I think, like ErrorPage is doing. It could probably be done in another branch though. Please merge!

comment:9 Changed 8 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [27539]) Merge wsgi-error-handling-4019

Author: exarkun Reviewer: therve Fixes: #4019

In the Twisted Web WSGI container, handle exceptions raised by the application object by turning them into 500 responses where possible or by closing the connection when it is too late to set the response code.

comment:10 Changed 8 years ago by osuchw

Thanks for working on this. Now I know how it should have been done. Live and learn :)

comment:11 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.