Added "sensible default error handling", auto-calc of Content-Length, and

various documentation enhancements.  Handlers will now log exceptions to
'wsgi.errors' by default, and generate a trivial client error message
(that can easily be changed in subclasses).  Added tests for "error before
headers sent" and "error after headers sent" scenarios.


git-svn-id: svn://svn.eby-sarna.com/svnroot/wsgiref@251 571e12c6-e1fa-0310-aee7-ff1267fa46bd

2 files changed, 213 insertions, 67 deletions

diff --git a/src/wsgiref/handlers.py b/src/wsgiref/handlers.py
index d49b687..287e3bb 100644
--- a/src/wsgiref/handlers.py
+++ b/src/wsgiref/handlers.py
@@ -1,3 +1,5 @@
+"""Base classes for server/gateway implementations"""
+
 from types import StringType
 from util import FileWrapper, guess_scheme
 from headers import Headers
@@ -37,39 +39,37 @@ except NameError:
 
 
 
-
-
 class BaseHandler:
     """Manage the invocation of a WSGI application"""
 
+    # Configuration parameters; can override per-subclass or per-instance
     wsgi_version = (1,0)
     wsgi_multithread = True
     wsgi_multiprocess = True
     wsgi_run_once = False
 
-    # os_environ may be overridden at class or instance level, if desired
-    os_environ = dict(os.environ.items())   
+    # os_environ is used to supply configuration from the OS environment:
+    # by default it's a copy of 'os.environ' as of import time, but you can
+    # override this in e.g. your __init__ method.
+    os_environ = dict(os.environ.items())
 
     # Collaborator classes
     wsgi_file_wrapper = FileWrapper     # set to None to disable
     headers_class = Headers             # must be a Headers-like class
 
-    # State variables
+    # Error handling (also per-subclass or per-instance)
+    traceback_limit = None  # Print entire traceback to self.get_stderr()
+    error_status = "500 Dude, this is whack!"
+    error_headers = [('Content-Type','text/plain')]
+    error_body = "A server error occurred.  Please contact the administrator."
+    
+    # State variables (don't mess with these)
     status = result = None
     headers_sent = False
     headers = None
     bytes_sent = 0
 
 
-    def run(self, application):
-        """Invoke the application"""
-        try:
-            self.setup_environ()
-            self.result = application(self.environ, self.start_response)
-            self.finish_response()
-        except:
-            self.handle_error()
-            self.close()
 
 
 
@@ -80,6 +80,26 @@ class BaseHandler:
 
 
 
+    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.
+
+
     def setup_environ(self):
         """Set up the environment for one request"""
 
@@ -89,7 +109,7 @@ class BaseHandler:
         env['wsgi.input']        = self.get_stdin()
         env['wsgi.errors']       = self.get_stderr()
         env['wsgi.version']      = self.wsgi_version
-        env['wsgi.run_once']    = self.wsgi_run_once
+        env['wsgi.run_once']     = self.wsgi_run_once
         env['wsgi.url_scheme']   = self.get_scheme()
         env['wsgi.multithread']  = self.wsgi_multithread
         env['wsgi.multiprocess'] = self.wsgi_multiprocess
@@ -98,27 +118,22 @@ class BaseHandler:
             env['wsgi.file_wrapper'] = self.wsgi_file_wrapper
 
 
+
+
+
     def finish_response(self):
         """Send any iterable data, then close self and the iterable
 
         Subclasses intended for use in asynchronous servers will
-        probably want to redefine this method, such that it sets up
-        callbacks to iterate over the data, and to call 'self.close()'
-        when finished.
+        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.
         """
-        try:
-            try:
-                if not self.result_is_file() and not self.sendfile():
-                    for data in self.result:
-                        self.write(data)
-                    self.finish_content()
-            except:
-                self.handle_error()
-        finally:
-            self.close()
-
-
-
+        if not self.result_is_file() and not self.sendfile():
+            for data in self.result:
+                self.write(data)
+            self.finish_content()
+        self.close()
 
 
     def get_scheme(self):
@@ -126,9 +141,25 @@ class BaseHandler:
         return guess_scheme(self.environ)
 
 
+    def set_content_length(self):
+        """Compute Content-Length or switch to chunked encoding if possible"""
+        try:
+            blocks = len(self.result)
+        except (TypeError,AttributeError,NotImplementedError):
+            pass
+        else:
+            if blocks==1:
+                self.headers['Content-Length'] = str(self.bytes_sent)
+                return
+        # XXX Try for chunked encoding if enabled
+        
+
     def cleanup_headers(self):
         """Make any necessary header changes or defaults"""
-        # XXX set up Content-Length, chunked encoding, if possible/needed
+        if not self.headers.has_key('Content-Length'):
+            self.set_content_length()
+
+        # XXX set up Date, Server headers 
 
 
     def start_response(self, status, headers,exc_info=None):
@@ -162,6 +193,16 @@ class BaseHandler:
 
 
 
+
+
+
+
+
+
+
+
+
+
     def write(self, data):
         """'write()' callable as specified by PEP 333"""
 
@@ -171,10 +212,12 @@ class BaseHandler:
              raise AssertionError("write() before start_response()")
 
         elif not self.headers_sent:
-             # Before the first output, send the stored headers
-             self.send_headers()
+            # Before the first output, send the stored headers
+            self.bytes_sent = len(data)    # make sure we know content-length
+            self.send_headers()
+        else:
+            self.bytes_sent += len(data)
 
-        self.bytes_sent += len(data)
         # XXX check Content-Length and truncate if too many bytes written?
         self._write(data)
         self._flush()
@@ -188,21 +231,19 @@ class BaseHandler:
         return iterable ('self.result') is an instance of
         'self.wsgi_file_wrapper'.
 
-        This method should return a true value if it is able to
+        This method should return a true value if it was able to actually
         transmit the wrapped file-like object using a platform-specific
         approach.  It should return a false value if normal iteration
         should be used instead.  An exception can be raised to indicate
         that transmission was attempted, but failed.
 
-        NOTE: this method should call 'self.send_headers()' if it is
-        going to attempt direct transmission, and 'self.headers_sent'
-        is false.
+        NOTE: this method should call 'self.send_headers()' if
+        'self.headers_sent' is false and it is going to attempt direct
+        transmission of the file1.
         """
         return False   # No platform-specific transmission by default
 
 
-
-
     def finish_content(self):
         """Ensure headers and content have both been sent"""
         if not self.headers_sent:
@@ -212,15 +253,16 @@ class BaseHandler:
             pass # XXX check if content-length was too short?
 
     def close(self):
-        """Close the iterable, if needed, and reset all instance vars
+        """Close the iterable (if needed) and reset all instance vars
 
         Subclasses may want to also drop the client connection.
         """
-        if hasattr(self.result,'close'):
-            self.result.close()
-        self.result = self.headers = self.status = self.environ = None
-        self.bytes_sent = 0
-        self.headers_sent = False
+        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_status(self):
@@ -229,7 +271,6 @@ class BaseHandler:
         (BaseCGIHandler overrides this to use a "Status:" prefix.)"""
         self._write('%s\r\n' % status)
 
-
     def send_headers(self):
         """Transmit headers to the client, via self._write()"""
         self.cleanup_headers()
@@ -244,19 +285,64 @@ class BaseHandler:
         return wrapper is not None and isinstance(self.result,wrapper)
 
 
-    # Pure abstract methods; *must* be overridden in subclasses
+    def log_exception(self,exc_info):
+        """Log the 'exc_info' tuple in the server log
+
+        Subclasses may override to retarget the output or change its format.
+        """
+        try:
+            from traceback import print_exception
+            print_exception(
+                exc_info[0], exc_info[1], exc_info[2],
+                self.traceback_limit, self.get_stderr()
+            )
+        finally:
+            exc_info = None
+
 
     def handle_error(self):
-        """Override in subclass to handle error recovery and logging"""
-        # XXX this really should do something sensible by default
-        raise NotImplementedError
+        """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?
+
+    def error_output(self, environ, start_response):
+        """WSGI mini-app to create error output
+
+        By default, this just uses the 'error_status', 'error_headers',
+        and 'error_body' attributes to generate an output page.  It can
+        be overridden in a subclass to dynamically generate diagnostics,
+        choose an appropriate message for the user's preferred language, etc.
+
+        Note, however, that it's not recommended from a security perspective to
+        spit out diagnostics to any old user; ideally, you should have to do
+        something special to enable diagnostic output, which is why we don't
+        include any here!
+        """
+        start_response(self.error_status, self.error_headers[:])
+        return [self.error_body]    
+
+
 
+    # Pure abstract methods; *must* be overridden in subclasses
+    
     def _write(self,data):
-        """Override in subclass to buffer data for send to client"""
+        """Override in subclass to buffer data for send to client
+
+        It's okay if this method actually transmits the data; BaseHandler
+        just separates write and flush operations for greater efficiency
+        when the underlying system actually has such a distinction.
+        """
         raise NotImplementedError
 
     def _flush(self):
-        """Override in subclass to force sending of recent '_write()' calls"""
+        """Override in subclass to force sending of recent '_write()' calls
+
+        It's okay if this method is a no-op (i.e., if '_write()' actually
+        sends the data.
+        """
         raise NotImplementedError
 
     def get_stdin(self):
@@ -281,10 +367,6 @@ class BaseHandler:
 
 
 
-
-
-
-
 class BaseCGIHandler(BaseHandler):
     """CGI-like systems using input/output/error streams and environ mapping
 
diff --git a/src/wsgiref/tests/test_handlers.py b/src/wsgiref/tests/test_handlers.py
index 69b86d9..7d0863f 100644
--- a/src/wsgiref/tests/test_handlers.py
+++ b/src/wsgiref/tests/test_handlers.py
@@ -1,11 +1,12 @@
 from __future__ import nested_scopes    # Backward compat for 2.1
 from unittest import TestCase, TestSuite, makeSuite
 from wsgiref.util import setup_testing_defaults
+from wsgiref.headers import Headers
 from wsgiref.handlers import BaseHandler, BaseCGIHandler
 from StringIO import StringIO
 
 
-class TestHandler(BaseCGIHandler):
+class ErrorHandler(BaseCGIHandler):
     """Simple handler subclass for testing BaseHandler"""
 
     def __init__(self,**kw):
@@ -15,6 +16,9 @@ class TestHandler(BaseCGIHandler):
             multithread=True, multiprocess=True
         )
 
+class TestHandler(ErrorHandler):
+    """Simple handler subclass for testing BaseHandler, w/error passthru"""
+
     def handle_error(self):
         raise   # for testing, we want to see what's happening
 
@@ -35,10 +39,6 @@ class TestHandler(BaseCGIHandler):
 
 
 
-
-
-
-
 class HandlerTests(TestCase):
 
     def checkEnvironAttrs(self, handler):
@@ -83,21 +83,85 @@ class HandlerTests(TestCase):
     def testAbstractMethods(self):
         h = BaseHandler()
         for name in [
-            'handle_error','_flush','get_stdin','get_stderr','add_cgi_vars'
+            '_flush','get_stdin','get_stderr','add_cgi_vars'
         ]:
             self.assertRaises(NotImplementedError, getattr(h,name))
         self.assertRaises(NotImplementedError, h._write, "test")
 
 
-    def testSimpleRun(self):
+    def testContentLength(self):
+        # Demo one reason iteration is better than write()...  ;)
+
+        def trivial_app1(e,s):
+            s('200 OK',[])
+            return [e['wsgi.url_scheme']]
+
+        def trivial_app2(e,s):
+            s('200 OK',[])(e['wsgi.url_scheme'])
+            return []
+
         h = TestHandler()
-        h.run(lambda e,s: [(s('200 OK',[]) or 1) and e['wsgi.url_scheme']])
-        self.assertEqual(h.stdout.getvalue(),"Status: 200 OK\r\n\r\nhttp")
+        h.run(trivial_app1)
+        self.assertEqual(h.stdout.getvalue(),
+            "Status: 200 OK\r\n"
+            "Content-Length: 4\r\n"
+            "\r\n"
+            "http")
         
+        h = TestHandler()
+        h.run(trivial_app2)
+        self.assertEqual(h.stdout.getvalue(),
+            "Status: 200 OK\r\n"
+            "\r\n"
+            "http")
+        
+
+
 
 
 
 
+    def testBasicErrorOutput(self):
+        
+        def non_error_app(e,s):
+            s('200 OK',[])
+            return []
+
+        def error_app(e,s):
+            raise AssertionError("This should be caught by handler")
+
+        h = ErrorHandler()
+        h.run(non_error_app)
+        self.assertEqual(h.stdout.getvalue(),
+            "Status: 200 OK\r\n"
+            "Content-Length: 0\r\n"
+            "\r\n")
+        self.assertEqual(h.stderr.getvalue(),"")
+
+        h = ErrorHandler()
+        h.run(error_app)
+        self.assertEqual(h.stdout.getvalue(),
+            "Status: %s\r\n"
+            "Content-Type: text/plain\r\n"
+            "Content-Length: %d\r\n"
+            "\r\n%s" % (h.error_status,len(h.error_body),h.error_body))
+
+        self.failUnless(h.stderr.getvalue().find("AssertionError")<>-1)
+
+    def testErrorAfterOutput(self):
+        MSG = "Some output has been sent"
+        def error_app(e,s):
+            s("200 OK",[])(MSG)
+            raise AssertionError("This should be caught by handler")
+
+        h = ErrorHandler()
+        h.run(error_app)
+        self.assertEqual(h.stdout.getvalue(),
+            "Status: 200 OK\r\n"
+            "\r\n"+MSG)
+        self.failUnless(h.stderr.getvalue().find("AssertionError")<>-1)
+
+
 
 TestClasses = (
     HandlerTests,