| msg172830 - (view) |
Author: Brent Tubbs (btubbs) |
Date: 2012-10-13 21:00 |
| When a WSGI application returns an iterable that has a .close() method, the server is supposed to call that method once the request has finished. The wsgiref server does not do this when a client disconnects from a streaming response. The attached script allows testing the .close() behavior of various wsgi servers (wsgiref, cherrypy, gevent, werkzeug, and gunicorn). wsgiref is the only one of the tested implementations that does not call .close(). |
|
|
| msg172831 - (view) |
Author: Antoine Pitrou (pitrou) *  |
Date: 2012-10-13 21:14 |
| This sounds like a reasonable expectation. Do you want to provide a patch? |
|
|
| msg172841 - (view) |
Author: PJ Eby (pje) * |
Date: 2012-10-14 00:26 |
| FYI, this looks like a bug in wsgiref.handlers.BaseHandler.finish_response(), which should probably be using a try/finally to ensure .close() gets called. (Which would catch a failed write() to the client.) I'm kind of amazed this has gone undetected this long, but I guess that either: 1. other servers are probably catching errors from .run() and closing, 2. they're not using BaseHandler in their implementation, or 3. most apps don't have anything that essential in close() and/or the clients don't disconnect that often. ;-) |
|
|
| msg172858 - (view) |
Author: Brent Tubbs (btubbs) |
Date: 2012-10-14 08:41 |
| Patch attached. I ran into this while trying to figure out why close() wasn't being called while running the Django dev server, which inherits from wsgiref's BaseHandler. So I'm also surprised that it's gone unnoticed so long. Thanks for the quick attention and encouragement on this! |
|
|
| msg172937 - (view) |
Author: Jesús Cea Avión (jcea) * |
Date: 2012-10-15 02:22 |
| Brent, could you possibly provide also a test?. Something to integrate with the standard testsuite that fails without your patch but passes with it. |
|
|
| msg172939 - (view) |
Author: Jesús Cea Avión (jcea) * |
Date: 2012-10-15 02:30 |
| Your patch is trivial enough, I am OK to integrate it as is, but it would be nice to verify that we are not reintroducing this bug in the future. I know that writing a the test will be far more difficult that writing the patch. You can try :). Anyway I am OK to integrate current patch as is. Looks simple enough. The comment in "run()" is a bit scary, and we could call "self.close()" twice, with this patch, or call extra code after "self.close()" is called. I don't know it that would be an issue, I am not familiarized enough with WSGI. |
|
|
| msg172940 - (view) |
Author: Graham Dumpleton (grahamd) |
Date: 2012-10-15 02:40 |
| Hmmm. Wonders if finally finding this was prompted in part by recent post about this very issue. :-) http://blog.dscpl.com.au/2012/10/obligations-for-calling-close-on.html Also related is this issue from Django I highlighted long time ago. https://code.djangoproject.com/ticket/16241 I would have to look through Django code again but wondering if the issue there was in fact caused by underlying issue in standard library or whether would have needed to be separately fixed in Django still. |
|
|
| msg172945 - (view) |
Author: Brent Tubbs (btubbs) |
Date: 2012-10-15 05:19 |
| You guessed it Graham! Bob Brewer pointed me to your post while I was fighting with this, which led me to testing the behavior under various servers and finding the wsgiref issue. Current Django trunk doesn't have its own finish_response anymore for the dev server; it's using the one on wsgiref's BaseHandler. So Django's problem should get fixed when wsgiref's does. I'm working on a test of the simpler case of ensuring that close() is called when any old exception is raised during the response. A test that actually sets up a socket and tests the client disconnecting would be a lot more complicated and a bit out of step with the current wsgi tests that all seem to use a socket-less mock. |
|
|
| msg172952 - (view) |
Author: Graham Dumpleton (grahamd) |
Date: 2012-10-15 09:11 |
| That's right, the Django bug report I filed was actually for Django 1.3, which didn't use wsgiref. I wasn't using Django 1.4 at the time so didn't bother to check its new implementation based on wsgiref. Instead I just assumed wsgiref would be right. Whoops. |
|
|
| msg173234 - (view) |
Author: Brent Tubbs (btubbs) |
Date: 2012-10-18 06:32 |
| Updated patch with test attached. |
|
|
| msg173442 - (view) |
Author: Roundup Robot (python-dev)  |
Date: 2012-10-21 12:11 |
| New changeset d5af1b235dab by Antoine Pitrou in branch '2.7': Issue #16220: wsgiref now always calls close() on an iterable response. http://hg.python.org/cpython/rev/d5af1b235dab |
|
|
| msg173445 - (view) |
Author: Roundup Robot (python-dev) |
Date: 2012-10-21 12:17 |
| New changeset eef470032457 by Antoine Pitrou in branch '3.2': Issue #16220: wsgiref now always calls close() on an iterable response. http://hg.python.org/cpython/rev/eef470032457 New changeset 2530acc092d8 by Antoine Pitrou in branch '3.3': Issue #16220: wsgiref now always calls close() on an iterable response. http://hg.python.org/cpython/rev/2530acc092d8 New changeset cf3a739345c6 by Antoine Pitrou in branch 'default': Issue #16220: wsgiref now always calls close() on an iterable response. http://hg.python.org/cpython/rev/cf3a739345c6 |
|
|
| msg173446 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2012-10-21 12:17 |
| Your patch is now committed, Brent, thank you! |
|
|