launchpad-reviewers team mailing list archive

[William Grant <me@xxxxxxxxxxxxxxxxxx>]

William Grant has proposed merging ~wgrant/turnip:http-errors into turnip:master.

Commit message:
Clean up HTTP and pack protocol error reporting.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~wgrant/turnip/+git/turnip/+merge/281593

Clean up HTTP and pack protocol error reporting.

The various HTTP error methods are merged into a single function, and some additional error cases no longer hang connections. The pack protocol moves to the same pattern of having connectToBackend return a deferred that errbacks on connection failure.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~wgrant/turnip:http-errors into turnip:master.

diff --git a/turnip/pack/git.py b/turnip/pack/git.py
index 8dd6f5f..49bed5e 100644
--- a/turnip/pack/git.py
+++ b/turnip/pack/git.py
@@ -302,8 +302,9 @@ class PackClientFactory(protocol.ClientFactory):
 
     protocol = PackClientProtocol
 
-    def __init__(self, server):
+    def __init__(self, server, deferred):
         self.server = server
+        self.deferred = deferred
 
     def startedConnecting(self, connector):
         self.server.log.info(
@@ -315,8 +316,7 @@ class PackClientFactory(protocol.ClientFactory):
         return p
 
     def clientConnectionFailed(self, connector, reason):
-        self.server.log.failure('Backend connection failed.', failure=reason)
-        self.server.die(b'Backend connection failed.')
+        self.deferred.errback(reason)
 
 
 class PackProxyServerProtocol(PackServerProtocol):
@@ -334,9 +334,11 @@ class PackProxyServerProtocol(PackServerProtocol):
         self.command = command
         self.pathname = pathname
         self.params = params
-        client = self.client_factory(self)
+        d = defer.Deferred()
+        client = self.client_factory(self, d)
         reactor.connectTCP(
             self.factory.backend_host, self.factory.backend_port, client)
+        return d
 
     def resumeProducing(self):
         # Send our translated request and then open the gate to the
@@ -465,7 +467,11 @@ class PackVirtServerProtocol(PackProxyServerProtocol):
         except Exception as e:
             self.die(VIRT_ERROR_PREFIX + b'INTERNAL_SERVER_ERROR ' + str(e))
         else:
-            self.connectToBackend(command, pathname, params)
+            try:
+                yield self.connectToBackend(command, pathname, params)
+            except Exception as e:
+                self.server.log.failure('Backend connection failed.')
+                self.server.die(b'Backend connection failed.')
 
 
 class PackVirtFactory(protocol.Factory):
diff --git a/turnip/pack/http.py b/turnip/pack/http.py
index 2c45055..7e27578 100644
--- a/turnip/pack/http.py
+++ b/turnip/pack/http.py
@@ -67,6 +67,20 @@ if sys.version_info.major < 3:
     from twisted.web import xmlrpc
 
 
+def fail_request(request, message, code=http.INTERNAL_SERVER_ERROR):
+    if not request.startedWriting:
+        request.setResponseCode(code)
+        request.setHeader(b'Content-Type', b'text/plain; charset=UTF-8')
+        if not isinstance(message, bytes):
+            message = message.encode('UTF-8')
+        request.write(message)
+    request.unregisterProducer()
+    request.finish()
+    # Some callsites want to be able to return from render_*, so make
+    # that possible.
+    return b''
+
+
 class HTTPPackClientProtocol(PackProtocol):
     """Abstract bridge between a Git pack connection and a smart HTTP request.
 
@@ -114,12 +128,7 @@ class HTTPPackClientProtocol(PackProtocol):
         if error_code is not None:
             # This is probably a system error (bad auth, not found,
             # repository corruption, etc.), so fail the request.
-            self.factory.http_request.setResponseCode(error_code)
-            self.factory.http_request.setHeader(
-                b'Content-Type', b'text/plain; charset=UTF-8')
-            self.factory.http_request.write(msg)
-            self.factory.http_request.unregisterProducer()
-            self.factory.http_request.finish()
+            fail_request(self.factory.http_request, msg, error_code)
             return True
         # We don't know it was a system error, so just send it back to
         # the client as a remote error and proceed to forward data
@@ -133,6 +142,7 @@ class HTTPPackClientProtocol(PackProtocol):
 
     def connectionMade(self):
         """Forward the request and the client's payload to the backend."""
+        self.factory.deferred.callback(None)
         self.factory.http_request.notifyFinish().addBoth(self._finish)
         self.factory.http_request.registerProducer(
             self.transport, True)
@@ -167,13 +177,8 @@ class HTTPPackClientProtocol(PackProtocol):
     def connectionLost(self, reason):
         self.factory.http_request.unregisterProducer()
         if not self.factory.http_request.finished:
-            if not self.raw:
-                self.factory.http_request.setResponseCode(
-                    http.INTERNAL_SERVER_ERROR)
-                self.factory.http_request.setHeader(
-                    b'Content-Type', b'text/plain')
-                self.factory.http_request.write(b'Backend connection lost.')
-            self.factory.http_request.finish()
+            fail_request(
+                self.factory.http_request, b'Backend connection lost.')
 
 
 class HTTPPackClientRefsProtocol(HTTPPackClientProtocol):
@@ -206,12 +211,16 @@ class HTTPPackClientCommandProtocol(HTTPPackClientProtocol):
 
 class HTTPPackClientFactory(protocol.ClientFactory):
 
-    def __init__(self, command, pathname, params, body, http_request):
+    def __init__(self, command, pathname, params, body, http_request, d):
         self.command = command
         self.pathname = pathname
         self.params = params
         self.body = body
         self.http_request = http_request
+        self.deferred = d
+
+    def clientConnectionFailed(self, connector, reason):
+        self.deferred.errback(reason)
 
 
 class HTTPPackClientCommandFactory(HTTPPackClientFactory):
@@ -229,20 +238,12 @@ class BaseSmartHTTPResource(resource.Resource):
 
     extra_params = {}
 
-    def errback(self, failure, request):
+    def errback(self, failure, request, msg):
         """Handle a Twisted failure by returning an HTTP error."""
-        failure.printTraceback()
+        log.err(failure, msg)
         if request.finished:
             return
-        request.write(self.error(request, repr(failure)))
-        request.unregisterProducer()
-        request.finish()
-
-    def error(self, request, message, code=http.INTERNAL_SERVER_ERROR):
-        """Prepare for an error response and return the body."""
-        request.setResponseCode(code)
-        request.setHeader(b'Content-Type', b'text/plain')
-        return message
+        fail_request(request, msg)
 
     @defer.inlineCallbacks
     def authenticateUser(self, request):
@@ -270,8 +271,10 @@ class BaseSmartHTTPResource(resource.Resource):
             params[b'turnip-authenticated-user'] = authenticated_user
             params[b'turnip-authenticated-uid'] = str(authenticated_uid)
         params.update(self.extra_params)
-        client_factory = factory(service, path, params, content, request)
+        d = defer.Deferred()
+        client_factory = factory(service, path, params, content, request, d)
         self.root.connectToBackend(client_factory)
+        yield d
 
 
 class SmartHTTPRefsResource(BaseSmartHTTPResource):
@@ -289,18 +292,18 @@ class SmartHTTPRefsResource(BaseSmartHTTPResource):
         try:
             service = request.args['service'][0]
         except (KeyError, IndexError):
-            return self.error(
+            return fail_request(
                 request, b'Only git smart HTTP clients are supported.',
                 code=http.NOT_FOUND)
 
         if service not in self.root.allowed_services:
-            return self.error(
+            return fail_request(
                 request, b'Unsupported service.', code=http.FORBIDDEN)
 
         d = self.connectToBackend(
             HTTPPackClientRefsFactory, service, self.path, request.content,
             request)
-        d.addErrback(self.errback, request)
+        d.addErrback(self.errback, request, b'Backend connection failed')
         return server.NOT_DONE_YET
 
 
@@ -319,7 +322,7 @@ class SmartHTTPCommandResource(BaseSmartHTTPResource):
     def render_POST(self, request):
         content_type = request.requestHeaders.getRawHeaders(b'Content-Type')
         if content_type != [b'application/x-%s-request' % self.service]:
-            return self.error(
+            return fail_request(
                 request, b'Invalid Content-Type for service.',
                 code=http.BAD_REQUEST)
 
@@ -335,7 +338,7 @@ class SmartHTTPCommandResource(BaseSmartHTTPResource):
         d = self.connectToBackend(
             HTTPPackClientCommandFactory, self.service, self.path, content,
             request)
-        d.addErrback(self.errback, request)
+        d.addErrback(self.errback, request, b'Backend connection failed')
         return server.NOT_DONE_YET
 
 
@@ -465,11 +468,6 @@ class BaseHTTPAuthResource(resource.Resource):
         request.setResponseCode(code)
         request.setHeader(b'Content-Type', b'text/plain')
 
-    def _error(self, request, message, code=http.INTERNAL_SERVER_ERROR):
-        self._setErrorCode(request, code=code)
-        request.write(message.encode('UTF-8'))
-        request.finish()
-
     def _makeConsumer(self, session):
         """Build an OpenID `Consumer` object with standard arguments."""
         # Multiple instances need to share a store or not use one at all (in
@@ -578,30 +576,28 @@ class HTTPAuthRootResource(BaseHTTPAuthResource):
 
     def _translatePathCallback(self, translated, request):
         if 'path' not in translated:
-            self._error(request, 'translatePath response did not include path')
-            return
+            return fail_request(
+                request, 'translatePath response did not include path')
         repo_url = request.path.rstrip('/')
         # cgit simply parses configuration values up to the end of a line
         # following the first '=', so almost anything is safe, but
         # double-check that there are no newlines to confuse things.
         if '\n' in repo_url:
-            self._error(request, 'repository URL may not contain newlines')
-            return
+            return fail_request(
+                request, 'repository URL may not contain newlines')
         try:
             repo_path = compose_path(self.root.repo_store, translated['path'])
         except ValueError as e:
-            self._error(request, str(e))
-            return
+            return fail_request(request, str(e))
         trailing = translated.get('trailing')
         if trailing:
             if not trailing.startswith('/'):
                 trailing = '/' + trailing
             if not repo_url.endswith(trailing):
-                self._error(
+                return fail_request(
                     request,
                     'translatePath returned inconsistent response: '
                     '"%s" does not end with "%s"' % (repo_url, trailing))
-                return
             repo_url = repo_url[:-len(trailing)]
         repo_url = repo_url.strip('/')
         cgit_resource = CGitScriptResource(
@@ -609,13 +605,15 @@ class HTTPAuthRootResource(BaseHTTPAuthResource):
         request.render(cgit_resource)
 
     def _translatePathErrback(self, failure, request, session):
-        e = failure.value
-        message = e.faultString
-        if e.faultCode in (1, 290):
+        if failure.check(xmlrpc.Fault) is not None:
+            code, message = failure.value.faultCode, failure.value.faultString
+        else:
+            code, message = None, 'Unexpected error in translatePath.'
+        if code in (1, 290):
             error_code = http.NOT_FOUND
-        elif e.faultCode in (2, 310):
+        elif code in (2, 310):
             error_code = http.FORBIDDEN
-        elif e.faultCode in (3, 410):
+        elif code in (3, 410):
             if 'user' in session:
                 error_code = http.FORBIDDEN
                 message = (
@@ -628,8 +626,9 @@ class HTTPAuthRootResource(BaseHTTPAuthResource):
                 self._beginLogin(request, session)
                 return
         else:
+            log.err(failure, "Unexpected error in translatePath")
             error_code = http.INTERNAL_SERVER_ERROR
-        self._error(request, message, code=error_code)
+        fail_request(request, message, code=error_code)
 
     def render_GET(self, request):
         session = self._getSession(request)
diff --git a/turnip/pack/tests/test_http.py b/turnip/pack/tests/test_http.py
index ac9c42e..bc3bf10 100644
--- a/turnip/pack/tests/test_http.py
+++ b/turnip/pack/tests/test_http.py
@@ -24,10 +24,16 @@ from turnip.pack import (
 
 class LessDummyRequest(requesthelper.DummyRequest):
 
+    startedWriting = 0
+
     @property
     def value(self):
         return "".join(self.written)
 
+    def write(self, data):
+        self.startedWriting = 1
+        super(LessDummyRequest, self).write(data)
+
     def registerProducer(self, prod, s):
         # Avoid DummyRequest.registerProducer calling resumeProducing
         # forever, never giving the reactor a chance to run.