Opened 13 years ago

Closed 11 years ago

Last modified 11 years ago

#16241 closed Bug (fixed)

Django runserver (<=1.3) is not WSGI compliant.

Reported by: Graham Dumpleton Owned by: Aymeric Augustin
Component: Core (Other) Version: 1.3
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The Django runserver doesn't call close() on the iterable returned from WSGI application in all circumstances which is a violation of WSGI specification.

The specific scenario where close() is not called is where a socket error occurs writing response content after headers have been written. For example for:

Traceback (most recent call last):
  File "/Library/WebServer/Sites/django-4/lib/python2.6/site-packages/Django-1.3-py2.6.egg/django/core/servers/basehttp.py", line 284, in run
    self.finish_response()
  File "/Library/WebServer/Sites/django-4/lib/python2.6/site-packages/Django-1.3-py2.6.egg/django/core/servers/basehttp.py", line 321, in finish_response
    self.write(data)
  File "/Library/WebServer/Sites/django-4/lib/python2.6/site-packages/Django-1.3-py2.6.egg/django/core/servers/basehttp.py", line 400, in write
    self.send_headers()
  File "/Library/WebServer/Sites/django-4/lib/python2.6/site-packages/Django-1.3-py2.6.egg/django/core/servers/basehttp.py", line 465, in send_headers
    self.send_preamble()
  File "/Library/WebServer/Sites/django-4/lib/python2.6/site-packages/Django-1.3-py2.6.egg/django/core/servers/basehttp.py", line 382, in send_preamble
    'Date: %s\r\n' % http_date()
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/socket.py", line 297, in write
    self.flush()
  File "/System/Library/Frameworks/Python.framework/Versions/2.6/lib/python2.6/socket.py", line 284, in flush
    self._sock.sendall(buffer)
error: [Errno 32] Broken pipe

This error arrises in:

    def finish_response(self):
        """
        Send any iterable data, then close self and the iterable

        Subclasses intended for use in asynchronous servers will want to
        redefine this method, such that it sets up callbacks in the event loop
        to iterate over the data, and to call 'self.close()' once the response
        is finished.
        """
        if not self.result_is_file() or not self.sendfile():
            for data in self.result:
                self.write(data)
            self.finish_content()
        self.close()

Ie., the write() call fails. The 'self.close()' at this point is bypassed and so close() on iterable is not called.

The second chance where could be called is handle_error() as called within except block of:

    def run(self, application):
        """Invoke the application"""
        # Note to self: don't move the close()!  Asynchronous servers shouldn't
        # call close() from finish_response(), so if you close() anywhere but
        # the double-error branch here, you'll break asynchronous servers by
        # prematurely closing.  Async servers must return from 'run()' without
        # closing if there might still be output to iterate over.
        try:
            self.setup_environ()
            self.result = application(self.environ, self.start_response)
            self.finish_response()
        except:
            try:
                self.handle_error()
            except:
                # If we get an error handling an error, just give up already!
                self.close()
                raise   # ...and let the actual server figure it out.

The code for that though is:

    def handle_error(self):
        """Log current error, and send error output to client if possible"""
        self.log_exception(sys.exc_info())
        if not self.headers_sent:
            self.result = self.error_output(self.environ, self.start_response)
            self.finish_response()
        # XXX else: attempt advanced recovery techniques for HTML or text?

Because though headers are already sent then 'self.close()' not called there either.

End result is that if application returned a custom iterable in some way, close() isn't called on it, meaning that one could have a leakage of resources when socket error occurs.

Code for django.core.servers.basehttp should be patched something like:

*** /Users/graham/Packages/Django-1.3/django/core/servers/basehttp.py	2011-03-02 21:40:48.000000000 +1100
--- /Library/WebServer/Sites/django-4/lib/python2.6/site-packages/Django-1.3-py2.6.egg/django/core/servers/basehttp.py	2011-06-13 17:21:56.000000000 +1000
***************
*** 283,294 ****
              self.result = application(self.environ, self.start_response)
              self.finish_response()
          except:
!             try:
!                 self.handle_error()
!             except:
!                 # If we get an error handling an error, just give up already!
!                 self.close()
!                 raise   # ...and let the actual server figure it out.
  
      def setup_environ(self):
          """Set up the environment for one request"""
--- 283,291 ----
              self.result = application(self.environ, self.start_response)
              self.finish_response()
          except:
!             self.handle_error()
!         finally:
!             self.close()
  
      def setup_environ(self):
          """Set up the environment for one request"""
***************
*** 449,463 ****
              pass # XXX check if content-length was too short?
  
      def close(self):
!         try:
!             self.request_handler.log_request(self.status.split(' ',1)[0], self.bytes_sent)
!         finally:
              try:
!                 if hasattr(self.result,'close'):
!                     self.result.close()
              finally:
!                 self.result = self.headers = self.status = self.environ = None
!                 self.bytes_sent = 0; self.headers_sent = False
  
      def send_headers(self):
          """Transmit headers to the client, via self._write()"""
--- 446,461 ----
              pass # XXX check if content-length was too short?
  
      def close(self):
!         if self.result is not None:
              try:
!                 self.request_handler.log_request(self.status.split(' ',1)[0], self.bytes_sent)
              finally:
!                 try:
!                     if hasattr(self.result,'close'):
!                         self.result.close()
!                 finally:
!                     self.result = self.headers = self.status = self.environ = None
!                     self.bytes_sent = 0; self.headers_sent = False
  
      def send_headers(self):
          """Transmit headers to the client, via self._write()"""

Now, given that Django 1.4 does away with the internal implementation of a WSGI server and uses wsgiref instead, fully expect that this issue will be marked as 'will not fix' as no will care about it for older versions. :-)

This likely is also only going to be an issue where someone (like me) was screwing with things and wrapping the Django WSGI application object passed to the ServerHandler.run() method with some fancy WSGI middleware that was doing stuff that was dependent on close() always being called as it should be.

Thus, logged for posterity mostly.

Change History (7)

comment:1 by Aymeric Augustin, 13 years ago

Resolution: fixed
Status: newclosed
Triage Stage: UnreviewedAccepted

Indeed, this issue no longer exists in Django 1.4.

Per the new backport policy (http://groups.google.com/group/django-developers/browse_thread/thread/3f856c6bccea172a) we don't have to fix it in Django 1.3: it's not a security issue, nor a data-loss bug, nor a crashing bug. The development server is not designed for environments where the resource leak may become an issue, so the issue will only pop up in some edge cases, like yours.

Django used a copy of wsgiref; I assume you've checked if the issue still exists in the standard library, and if it does, filed a bug in Python's tracker?

comment:2 by Aymeric Augustin, 11 years ago

The underlying bug was fixed in Python 2.7 two months ago: http://bugs.python.org/issue16220

comment:3 by Aymeric Augustin, 11 years ago

Resolution: fixed
Status: closednew

In #19519 we're about to wire the request_finished signal in BaseHttpResponse.close().

If we do that, that bug in Python <= 2.7.3 will have consequences on a document feature of Django, which justifies backporting the fix from Python.

comment:4 by Aymeric Augustin, 11 years ago

Owner: changed from nobody to Aymeric Augustin
Status: newassigned

comment:5 by Aymeric Augustin, 11 years ago

Has patch: set

https://github.com/django/django/pull/612

(I will add the fix for #19519 to that pull request.)

comment:6 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In ac72782e61a8e08381cb66005295a4c24ed37f33:

[1.5.x] Fixed #16241 -- Ensured the WSGI iterable's close() is always called.

Thanks Graham Dumpleton for the report.

Backport of a53c474.

comment:7 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In a53c4740268721036a6b3bc73f5e8f557c18bac0:

Fixed #16241 -- Ensured the WSGI iterable's close() is always called.

Thanks Graham Dumpleton for the report.

Note: See TracTickets for help on using tickets.
Back to Top