Opened 5 years ago

Closed 5 years ago

Last modified 5 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: exarkun, therve Branch: branches/wsgi-error-handling-4019
(diff, github, buildbot, log)
Author: exarkun Launchpad Bug:

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 5 years ago.
Updated

Download all attachments as: .zip

Change History (12)

comment:1 Changed 5 years ago by exarkun

  • Cc exarkun 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 5 years ago by osuchw

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

Changed 5 years ago by osuchw

Updated

comment:3 Changed 5 years ago by osuchw

updated to work with current trunk

comment:4 Changed 5 years ago by exarkun

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 5 years ago by exarkun

  • Owner changed from osuchw to exarkun
  • Status changed from new to assigned

comment:6 Changed 5 years ago by exarkun

  • Author set to exarkun
  • Branch set to branches/wsgi-error-handling-4019

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

comment:7 Changed 5 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted
  • Status changed from assigned to new

comment:8 Changed 5 years ago by therve

  • Cc therve added
  • Keywords review removed
  • Owner set to exarkun

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 5 years ago by exarkun

  • Resolution set to fixed
  • Status changed from new to closed

(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 5 years ago by osuchw

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

comment:11 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.