launchpad-reviewers team mailing list archive

[John A Meinel <john@xxxxxxxxxxxxxxxxx>]

John A Meinel has proposed merging lp:~jameinel/launchpad/loggerhead-disconnect-701329 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  #701329 loggerhead OOPS - error: [Errno 32] Broken pipe
  https://bugs.launchpad.net/bugs/701329

For more details, see:
https://code.launchpad.net/~jameinel/launchpad/loggerhead-disconnect-701329/+merge/48665

This should fix the 16k OOPS reports that we were seeing on loggerhead. There are probably other ways to fix it, as well.

The failure was being triggered because haproxy is issuing a HEAD request to loggerhead, and then closing before it actually reads all of the result. loggerhead ends up getting socket.error(EPIPE) while trying to write information out.

I wasn't able to trigger this by going to the Apache proxy interface, presumably because Apache reads the full header from loggerhead before it then reports things back to the user.

In the WSGIHandler code, it is suppressing all of these socket errors (SSL error, socket.error, etc). I went ahead and followed suit, to suppress them instead of reporting an oops.

If you consider the code as it exists, it would want to give OOPS info to the user. However, if the socket is closed, you can't send anything anyway.

If we wanted, we could have a more fine-grained approach here (only suppress socket.error(EPIPE), or something like that.) We could also just trap at a different level. For example in the 'write_wrapper' function.
-- 
https://code.launchpad.net/~jameinel/launchpad/loggerhead-disconnect-701329/+merge/48665
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jameinel/launchpad/loggerhead-disconnect-701329 into lp:launchpad.

=== modified file 'lib/launchpad_loggerhead/app.py'
--- lib/launchpad_loggerhead/app.py	2011-01-31 23:45:55 +0000
+++ lib/launchpad_loggerhead/app.py	2011-02-04 21:17:23 +0000
@@ -18,6 +18,7 @@
 from openid.extensions.sreg import SRegRequest, SRegResponse
 from openid.consumer.consumer import CANCEL, Consumer, FAILURE, SUCCESS
 
+from paste import httpserver
 from paste.fileapp import DataApp
 from paste.request import construct_url, parse_querystring, path_info_pop
 from paste.httpexceptions import (
@@ -351,6 +352,12 @@
                 data = app_iter.next()
             except StopIteration:
                 return
+            except httpserver.SocketErrors, e:
+                # The Paste WSGIHandler suppresses these exceptions.
+                # Generally it means something like 'EPIPE' because the
+                # connection was closed. We don't want to generate an OOPS
+                # just because the connection was closed prematurely.
+                return
             except:
                 error_page_sent = wrapped.generate_oops(environ, error_utility)
                 if error_page_sent:

=== modified file 'lib/launchpad_loggerhead/tests.py'
--- lib/launchpad_loggerhead/tests.py	2010-10-04 19:50:45 +0000
+++ lib/launchpad_loggerhead/tests.py	2011-02-04 21:17:23 +0000
@@ -1,19 +1,24 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
+import errno
 import unittest
 import urllib
+import socket
 
 import lazr.uri
 import wsgi_intercept
 from wsgi_intercept.urllib2_intercept import install_opener, uninstall_opener
 import wsgi_intercept.zope_testbrowser
+from paste import httpserver
 from paste.httpexceptions import HTTPExceptionHandler
+import zope.event
 
 from canonical.config import config
+from canonical.launchpad.webapp.errorlog import ErrorReport, ErrorReportEvent
 from canonical.launchpad.webapp.vhosts import allvhosts
 from canonical.testing.layers import DatabaseFunctionalLayer
-from launchpad_loggerhead.app import RootApp
+from launchpad_loggerhead.app import RootApp, oops_middleware
 from launchpad_loggerhead.session import SessionHandler
 from lp.testing import TestCase
 
@@ -142,5 +147,61 @@
                          'This is a dummy destination.\n')
 
 
+class TestOopsMiddleware(TestCase):
+
+    def _eventTriggered(self, event):
+        if isinstance(event, ErrorReportEvent):
+            self.error_events.append(event)
+
+    def watchForErrorReportEvent(self):
+        self.error_events = []
+        zope.event.subscribers.append(self._eventTriggered)
+        self.addCleanup(zope.event.subscribers.remove, self._eventTriggered)
+
+    def runtime_failing_app(self, environ, start_response):
+        if False:
+            yield None
+        raise RuntimeError('just a generic runtime error.')
+
+    def socket_failing_app(self, environ, start_response):
+        if False:
+            yield None
+        raise socket.error(errno.EPIPE, 'Connection closed')
+
+    def noop_start_response(self, status, response_headers, exc_info=None):
+        def noop_write(chunk):
+            pass
+        return noop_write
+
+    def wrap_and_run(self, app):
+        self.watchForErrorReportEvent()
+        app = oops_middleware(app)
+        # Just random env data, rather than setting up a whole wsgi stack just
+        # to pass in values for this dict
+        environ = {'wsgi.version': (1, 0),
+                   'wsgi.url_scheme': 'http',
+                   'PATH_INFO': '/test/path',
+                   'REQUEST_METHOD': 'GET',
+                   'SERVER_NAME': 'localhost',
+                   'SERVER_PORT': '8080',
+                  }
+        result = list(app(environ, self.noop_start_response))
+        return result
+
+    def test_exception_triggers_oops(self):
+        res = self.wrap_and_run(self.runtime_failing_app)
+        # After the exception was raised, we should also have gotten an oops
+        # event
+        self.assertEqual(1, len(self.error_events))
+        event = self.error_events[0]
+        self.assertIsInstance(event, ErrorReportEvent)
+        self.assertIsInstance(event.object, ErrorReport)
+        self.assertEqual('RuntimeError', event.object.type)
+
+    def test_ignores_socket_exceptions(self):
+        res = self.wrap_and_run(self.socket_failing_app)
+        self.assertEqual(0, len(self.error_events))
+
+
 def test_suite():
     return unittest.TestLoader().loadTestsFromName(__name__)