classification
Title: wsgiref does not call close() on iterable response
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.4, Python 3.3, Python 3.2, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: btubbs, grahamd, jcea, pitrou, pje, python-dev
Priority: normal Keywords: patch

Created on 2012-10-13 21:00 by btubbs, last changed 2012-10-21 12:17 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
sleepy_app.py btubbs, 2012-10-13 21:00 test script
wsgiref_close.patch btubbs, 2012-10-14 08:41 review
wsgiref_close_plus_test.patch btubbs, 2012-10-18 06:32 review
Messages (13)
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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) 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) * (Python committer) Date: 2012-10-21 12:17
Your patch is now committed, Brent, thank you!
History
Date User Action Args
2012-10-21 12:17:46pitrousetstatus: open -> closed
resolution: fixed
messages: + msg173446

stage: needs patch -> resolved
2012-10-21 12:17:12python-devsetmessages: + msg173445
2012-10-21 12:11:10python-devsetnosy: + python-dev
messages: + msg173442
2012-10-18 06:32:25btubbssetfiles: + wsgiref_close_plus_test.patch

messages: + msg173234
2012-10-15 09:11:40grahamdsetmessages: + msg172952
2012-10-15 05:19:58btubbssetmessages: + msg172945
2012-10-15 02:40:05grahamdsetnosy: + grahamd
messages: + msg172940
2012-10-15 02:30:37jceasetmessages: + msg172939
2012-10-15 02:22:50jceasetmessages: + msg172937
2012-10-15 02:20:54jceasetnosy: + jcea
2012-10-14 08:41:17btubbssetfiles: + wsgiref_close.patch
keywords: + patch
messages: + msg172858
2012-10-14 00:26:46pjesetmessages: + msg172841
2012-10-13 21:14:29pitrousetversions: + Python 3.2, Python 3.3, Python 3.4
nosy: + pje, pitrou

messages: + msg172831

type: behavior
stage: needs patch
2012-10-13 21:01:14btubbssettitle: wsgiref does not call close() on iterable response when client -> wsgiref does not call close() on iterable response
2012-10-13 21:00:20btubbscreate