launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/turnip:large-http-push into turnip:master.

Commit message:
Fix large HTTP pushes

When doing a large push, git-remote-http optimises by first probing to
find out whether it is permitted to write to the repository before
sending large packfile data.  It does this by sending a request
containing just a flush-pkt, which causes git-receive-pack to exit
successfully with no output.

We therefore need to handle the case where the backend exits before
producing any output.  To avoid weakening our error handling too much,
we do this only if the connection is lost with ConnectionDone, and we
synthesise an error message from the backend if the git service process
exits non-zero with no output.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~cjwatson/turnip/+git/turnip/+merge/310117
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/turnip:large-http-push into turnip:master.

diff --git a/turnip/pack/git.py b/turnip/pack/git.py
index 04d6770..0e5d928 100644
--- a/turnip/pack/git.py
+++ b/turnip/pack/git.py
@@ -271,7 +271,13 @@ class GitProcessProtocol(protocol.ProcessProtocol):
     def loseWriteConnection(self):
         self.transport.closeChildFD(0)
 
-    def processEnded(self, status):
+    def processEnded(self, reason):
+        if reason.check(error.ProcessDone) and reason.value.exitCode != 0:
+            code = reason.value.exitCode
+            self.peer.log.info(
+                'git exited {code} with no output; synthesising an error',
+                code=code)
+            self.peer.sendPacket(ERROR_PREFIX + 'backend exited %d' % code)
         self.peer.transport.loseConnection()
 
     def pauseProducing(self):
diff --git a/turnip/pack/http.py b/turnip/pack/http.py
index ada6b5f..5e5f87e 100644
--- a/turnip/pack/http.py
+++ b/turnip/pack/http.py
@@ -32,6 +32,7 @@ from paste.auth.cookie import (
     )
 from twisted.internet import (
     defer,
+    error,
     protocol,
     reactor,
     )
@@ -98,9 +99,13 @@ class HTTPPackClientProtocol(PackProtocol):
 
     user_error_possible = True
 
+    def startGoodResponse(self):
+        """Prepare the HTTP response for forwarding from the backend."""
+        raise NotImplementedError()
+
     def backendConnected(self):
         """Called when the backend is connected and has sent a good packet."""
-        raise NotImplementedError()
+        self.startGoodResponse()
 
     def backendConnectionFailed(self, msg):
         """Called when the backend fails to connect or returns an error."""
@@ -179,8 +184,16 @@ class HTTPPackClientProtocol(PackProtocol):
     def connectionLost(self, reason):
         self.factory.http_request.unregisterProducer()
         if not self.factory.http_request.finished:
-            fail_request(
-                self.factory.http_request, b'Backend connection lost.')
+            if reason.check(error.ConnectionDone):
+                # We assume that the backend will have sent an error if
+                # necessary; otherwise an empty response is permitted (and
+                # needed by git's probe_rpc mechanism).
+                if not self.paused and not self.raw:
+                    self.startGoodResponse()
+                self.factory.http_request.finish()
+            else:
+                fail_request(
+                    self.factory.http_request, b'Backend connection lost.')
 
 
 class HTTPPackClientRefsProtocol(HTTPPackClientProtocol):
@@ -190,12 +203,15 @@ class HTTPPackClientRefsProtocol(HTTPPackClientProtocol):
     # failure from repository corruption or similar.
     user_error_possible = False
 
-    def backendConnected(self):
+    def startGoodResponse(self):
         """Prepare the HTTP response for forwarding from the backend."""
         self.factory.http_request.setResponseCode(http.OK)
         self.factory.http_request.setHeader(
             b'Content-Type',
             b'application/x-%s-advertisement' % self.factory.command)
+
+    def backendConnected(self):
+        HTTPPackClientProtocol.backendConnected(self)
         self.rawDataReceived(
             encode_packet(b'# service=%s\n' % self.factory.command))
         self.rawDataReceived(encode_packet(None))
@@ -203,7 +219,7 @@ class HTTPPackClientRefsProtocol(HTTPPackClientProtocol):
 
 class HTTPPackClientCommandProtocol(HTTPPackClientProtocol):
 
-    def backendConnected(self):
+    def startGoodResponse(self):
         """Prepare the HTTP response for forwarding from the backend."""
         self.factory.http_request.setResponseCode(http.OK)
         self.factory.http_request.setHeader(
diff --git a/turnip/pack/tests/test_functional.py b/turnip/pack/tests/test_functional.py
index e30b59f..61fd5c4 100644
--- a/turnip/pack/tests/test_functional.py
+++ b/turnip/pack/tests/test_functional.py
@@ -11,6 +11,7 @@ from __future__ import (
 from collections import defaultdict
 import hashlib
 import os
+import random
 import shutil
 import stat
 import tempfile
@@ -269,6 +270,44 @@ class FunctionalTestMixin(object):
             output)
         self.assertIn(b'does not appear to be a git repository', output)
 
+    @defer.inlineCallbacks
+    def test_large_push(self):
+        # Test a large push, which behaves a bit differently with some
+        # frontends.  For example, when doing a large push, as an
+        # optimisation, git-remote-http first probes to find out whether it
+        # is permitted to write to the repository before sending large
+        # packfile data.  It does this by sending a request containing just
+        # a flush-pkt, which causes git-receive-pack to exit successfully
+        # with no output.
+        test_root = self.useFixture(TempDir()).path
+        clone1 = os.path.join(test_root, 'clone1')
+        clone2 = os.path.join(test_root, 'clone2')
+
+        # Push a commit large enough to generate a pack that exceeds git's
+        # allocated buffer for HTTP pushes, thereby triggering 'probe_rpc'.
+        yield self.assertCommandSuccess((b'git', b'clone', self.url, clone1))
+        yield self.assertCommandSuccess(
+            (b'git', b'config', b'user.name', b'Test User'), path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'config', b'user.email', b'test@xxxxxxxxxxx'),
+            path=clone1)
+        with open(os.path.join(clone1, 'bigfile'), 'w') as bigfile:
+            # Use random contents to defeat compression.
+            bigfile.write(bytearray(
+                random.getrandbits(8) for _ in range(1024 * 1024)))
+        yield self.assertCommandSuccess(
+            (b'git', b'add', b'bigfile'), path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'commit', b'-m', b'Add big file'), path=clone1)
+        yield self.assertCommandSuccess(
+            (b'git', b'push', b'--all', b'origin'), path=clone1)
+
+        # Clone it again and make sure it's there.
+        yield self.assertCommandSuccess((b'git', b'clone', self.url, clone2))
+        out = yield self.assertCommandSuccess(
+            (b'git', b'log', b'--oneline', b'-n', b'1'), path=clone2)
+        self.assertIn(b'Add big file', out)
+
 
 class TestBackendFunctional(FunctionalTestMixin, TestCase):
 
diff --git a/turnip/pack/tests/test_http.py b/turnip/pack/tests/test_http.py
index 8e0418f..45cafad 100644
--- a/turnip/pack/tests/test_http.py
+++ b/turnip/pack/tests/test_http.py
@@ -109,11 +109,10 @@ class ErrorTestMixin(object):
 
     @defer.inlineCallbacks
     def test_backend_immediately_dies(self):
-        # If the backend disappears before it says anything, that's an
-        # internal server error.
+        # If the backend disappears before it says anything, that's OK.
         yield self.performRequest('')
-        self.assertEqual(500, self.request.responseCode)
-        self.assertEqual('Backend connection lost.', self.request.value)
+        self.assertEqual(200, self.request.responseCode)
+        self.assertEqual('', self.request.value)
 
     @defer.inlineCallbacks
     def test_backend_virt_error(self):