launchpad-reviewers team mailing list archive

["Thiago F. Pappacena" <pappacena@xxxxxxxxx>]

Thiago F. Pappacena has proposed merging ~pappacena/launchpad:buildmaster-connection-pool-lock into launchpad:master.

Commit message:
Fixing deadlock on LimitedHTTPConnectionPool

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~pappacena/launchpad/+git/launchpad/+merge/388434

Previously, LimitedHTTPConnectionPool was deferring the release of a semaphore to the end of event loop when we were giving back a connection to the pool.

But the operation to put back a connection in the pool is not async (although it sets a timer to remove the connection from the pool after some inactive time). So, there is no real reason to defer the semaphore release. We should do it right away, even if there is any exception when putting the connection back to the pool.
-- 
Your team Launchpad code reviewers is requested to review the proposed merge of ~pappacena/launchpad:buildmaster-connection-pool-lock into launchpad:master.

diff --git a/lib/lp/buildmaster/interactor.py b/lib/lp/buildmaster/interactor.py
index c19e617..0a67057 100644
--- a/lib/lp/buildmaster/interactor.py
+++ b/lib/lp/buildmaster/interactor.py
@@ -130,12 +130,14 @@ class LimitedHTTPConnectionPool(HTTPConnectionPool):
         return d
 
     def _putConnection(self, key, connection):
-        super(LimitedHTTPConnectionPool, self)._putConnection(key, connection)
-        # Only release the semaphore in the next main loop iteration; if we
-        # release it here then the next request may start using this
-        # connection's parser before this request has quite finished with
-        # it.
-        self._reactor.callLater(0, self._semaphore.release)
+        try:
+            super(LimitedHTTPConnectionPool, self)._putConnection(
+                key, connection)
+        finally:
+            # We should immediately release the lock, even if an exception
+            # happened: if for some reason we could not put the connection
+            # back to the pool, it's fine to allow new connections anyway.
+            self._semaphore.release()
 
 
 _default_pool = None
diff --git a/lib/lp/buildmaster/tests/test_interactor.py b/lib/lp/buildmaster/tests/test_interactor.py
index 22d788c..c033673 100644
--- a/lib/lp/buildmaster/tests/test_interactor.py
+++ b/lib/lp/buildmaster/tests/test_interactor.py
@@ -38,6 +38,7 @@ from twisted.internet import (
     defer,
     reactor as default_reactor,
     )
+from twisted.internet.endpoints import HostnameEndpoint
 from twisted.internet.task import Clock
 from twisted.python.failure import Failure
 
@@ -71,6 +72,7 @@ from lp.buildmaster.tests.mock_slaves import (
     SlaveTestHelpers,
     WaitingSlave,
     )
+from lp.services.compat import mock
 from lp.services.config import config
 from lp.services.features.testing import FeatureFixture
 from lp.services.twistedsupport.testing import TReqFixture
@@ -751,6 +753,56 @@ class TestSlaveConnectionTimeouts(TestCase):
         return assert_fails_with(d, defer.CancelledError)
 
 
+class TestLimitedHTTPConnectionPool(TestCase):
+    layer = LaunchpadZopelessLayer
+    run_tests_with = AsynchronousDeferredRunTestForBrokenTwisted.make_factory(
+        timeout=5)
+
+    @defer.inlineCallbacks
+    def test_limited_http_connection_pool_lock(self):
+        """Make sure that the semaphore on LimitedHTTPConnectionPool
+        doesn't hold locks for longer than needed."""
+        reactor = default_reactor
+        limit = 1
+        pool = LimitedHTTPConnectionPool(reactor=reactor, limit=limit)
+        pool.retryAutomatically = False
+        pool.maxPersistentPerHost = limit
+
+        host = config.librarian.download_host
+        port = config.librarian.download_port
+        key = ("http", host, port)
+        endpoint = HostnameEndpoint(reactor, host, port)
+
+        # Now, we wait for one deferred at a time. While putting them back in
+        # the pool, we raise some error: that's where we make sure the
+        # semaphore will be released no matter what.
+        pool._connections[key] = []
+        last_conn = None
+        for _ in range(5):
+            conn = yield pool.getConnection(key, endpoint)
+            if last_conn is not None:
+                # Force the previous connection back to the pool, to make sure
+                # we will call loseConnection on it and raise exception.
+                pool._connections[key].append(last_conn)
+
+            try:
+                with mock.patch.object(
+                        conn.transport, "loseConnection") as mock_lose_conn:
+                    mock_lose_conn.side_effect = IOError("!!")
+                    yield pool._putConnection(key, conn)
+                    self.assertEqual(1, mock_lose_conn.call_count)
+            except:
+                last_conn = conn
+            else:
+                self.fail("No exception was raised when putting connection "
+                          "back into the pool.")
+
+        # We should have tokens left to use at this point.
+        self.assertEqual(limit, pool._semaphore.tokens)
+
+        reactor.disconnectAll()
+
+
 class TestSlaveWithLibrarian(WithScenarios, TestCaseWithFactory):
     """Tests that need more of Launchpad to run."""