launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging ~cjwatson/turnip:virt-mem-leak into turnip:master.

Commit message:
Fix leak in PackVirtServerProtocol.requestReceived

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1867028 in turnip: "turnip-pack-virt keeps OOMing"
  https://bugs.launchpad.net/turnip/+bug/1867028

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

PackClientFactory called its Deferred's errback on failure, but didn't callback on success.  As a result, PackVirtServerProtocol.requestReceived never returned in the successful case, causing significant memory leaks over time.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~cjwatson/turnip:virt-mem-leak into turnip:master.

diff --git a/turnip/pack/git.py b/turnip/pack/git.py
index e09e2b5..0aa46c3 100644
--- a/turnip/pack/git.py
+++ b/turnip/pack/git.py
@@ -350,6 +350,7 @@ class PackClientFactory(protocol.ClientFactory):
     def buildProtocol(self, *args, **kwargs):
         p = protocol.ClientFactory.buildProtocol(self, *args, **kwargs)
         p.setPeer(self.server)
+        self.deferred.callback(None)
         return p
 
     def clientConnectionFailed(self, connector, reason):
diff --git a/turnip/pack/tests/test_git.py b/turnip/pack/tests/test_git.py
index 42ffb1b..9084f8f 100644
--- a/turnip/pack/tests/test_git.py
+++ b/turnip/pack/tests/test_git.py
@@ -7,6 +7,7 @@ from __future__ import (
     unicode_literals,
     )
 
+import hashlib
 import os.path
 
 from fixtures import TempDir
@@ -17,7 +18,9 @@ from testtools.matchers import (
     Equals,
     MatchesListwise,
     )
+from testtools.twistedsupport import AsynchronousDeferredRunTest
 from twisted.internet import (
+    defer,
     reactor as default_reactor,
     task,
     )
@@ -213,15 +216,57 @@ class TestPackBackendProtocol(TestCase):
         self.assertKilledWith(b'Symbolic ref target may not contain " "')
 
 
+class DummyPackBackendFactory(git.PackBackendFactory):
+
+    test_protocol = None
+
+    def buildProtocol(self, *args, **kwargs):
+        self.test_protocol = git.PackBackendFactory.buildProtocol(
+            self, *args, **kwargs)
+        return self.test_protocol
+
+
 class TestPackVirtServerProtocol(TestCase):
     """Test the Git pack virt protocol."""
 
+    run_tests_with = AsynchronousDeferredRunTest.make_factory(timeout=5)
+
     def assertKilledWith(self, message):
         self.assertFalse(self.transport.connected)
         self.assertEqual(
             (b'ERR turnip virt error: ' + message + b'\n', b''),
             helpers.decode_packet(self.transport.value()))
 
+    @defer.inlineCallbacks
+    def test_translatePath(self):
+        root = self.useFixture(TempDir()).path
+        hookrpc_handler = MockHookRPCHandler()
+        hookrpc_sock = os.path.join(root, 'hookrpc_sock')
+        backend_factory = DummyPackBackendFactory(
+            root, hookrpc_handler, hookrpc_sock)
+        backend_factory.protocol = DummyPackBackendProtocol
+        backend_listener = default_reactor.listenTCP(0, backend_factory)
+        backend_port = backend_listener.getHost().port
+        self.addCleanup(backend_listener.stopListening)
+        virtinfo = FakeVirtInfoService(allowNone=True)
+        virtinfo_listener = default_reactor.listenTCP(0, server.Site(virtinfo))
+        virtinfo_port = virtinfo_listener.getHost().port
+        virtinfo_url = b'http://localhost:%d/' % virtinfo_port
+        self.addCleanup(virtinfo_listener.stopListening)
+        factory = git.PackVirtFactory(
+            b'localhost', backend_port, virtinfo_url, 5)
+        proto = git.PackVirtServerProtocol()
+        proto.factory = factory
+        self.transport = proto_helpers.StringTransportWithDisconnection()
+        self.transport.protocol = proto
+        proto.makeConnection(self.transport)
+        proto.pauseProducing()
+        proto.got_request = True
+        yield proto.requestReceived(b'git-upload-pack', b'/example', {})
+        self.assertEqual(
+            proto.pathname, hashlib.sha256(b'/example').hexdigest())
+        backend_factory.test_protocol.transport.loseConnection()
+
     def test_translatePath_timeout(self):
         root = self.useFixture(TempDir()).path
         hookrpc_handler = MockHookRPCHandler()