Never return external connections to the GlobalLock connection pool

Summary:
Ref T13627. If a lock fails, the connection may be returned to the pool, even if the connection is an external connection. Under old versions of MySQL, connection reuse can release other locks on the same connection.

Don't return external connections to the pool.

This issue was introduced in D21369.

Test Plan: Added a failing test and made it pass.

Maniphest Tasks: T13627

Differential Revision: https://secure.phabricator.com/D21585

src/infrastructure/util/PhabricatorGlobalLock.php

@@ -118,6 +118,28 @@ final class PhabricatorGlobalLock extends PhutilLock {
     self::$pool = array();
   }
 
+  public static function newConnection() {
+    // NOTE: Use of the "repository" database is somewhat arbitrary, mostly
+    // because the first client of locks was the repository daemons.
+
+    // We must always use the same database for all locks, because different
+    // databases may be on different hosts if the database is partitioned.
+
+    // However, we don't access any tables so we could use any valid database.
+    // We could build a database-free connection instead, but that's kind of
+    // messy and unusual.
+
+    $dao = new PhabricatorRepository();
+
+    // NOTE: Using "force_new" to make sure each lock is on its own connection.
+
+    // See T13627. This is critically important in versions of MySQL older
+    // than MySQL 5.7, because they can not hold more than one lock per
+    // connection simultaneously.
+
+    return $dao->establishConnection('w', $force_new = true);
+  }
+
 /* -(  Implementation  )----------------------------------------------------- */
 
   protected function doLock($wait) {
@@ -135,18 +157,7 @@ final class PhabricatorGlobalLock extends PhutilLock {
     }
 
     if (!$conn) {
-      // NOTE: Using the 'repository' database somewhat arbitrarily, mostly
-      // because the first client of locks is the repository daemons. We must
-      // always use the same database for all locks, but don't access any
-      // tables so we could use any valid database. We could build a
-      // database-free connection instead, but that's kind of messy and we
-      // might forget about it in the future if we vertically partition the
-      // application.
-      $dao = new PhabricatorRepository();
-
-      // NOTE: Using "force_new" to make sure each lock is on its own
-      // connection.
-      $conn = $dao->establishConnection('w', $force_new = true);
+      $conn = self::newConnection();
     }
 
     // See T13627. We must never hold more than one lock per connection, so
@@ -189,7 +200,12 @@ final class PhabricatorGlobalLock extends PhutilLock {
       // example, this can happen if there are a large number of webhook tasks
       // in the queue.
 
-      self::$pool[] = $conn;
+      // See T13627. If this is an external connection, don't put it into
+      // the shared connection pool.
+
+      if (!$this->externalConnection) {
+        self::$pool[] = $conn;
+      }
 
       throw id(new PhutilLockException($lock_name))
         ->setHint($this->newHint($lock_name, $wait));

src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php

@@ -37,8 +37,7 @@ final class PhabricatorGlobalLockTestCase
   }
 
   public function testConnectionPoolWithSpecificConnection() {
-    $conn = id(new HarbormasterScratchTable())
-      ->establishConnection('w');
+    $conn = PhabricatorGlobalLock::newConnection();
 
     PhabricatorGlobalLock::clearConnectionPool();
 
@@ -86,8 +85,7 @@ final class PhabricatorGlobalLockTestCase
   }
 
   public function testExternalConnectionMutationScope() {
-    $conn = id(new HarbormasterScratchTable())
-      ->establishConnection('w');
+    $conn = PhabricatorGlobalLock::newConnection();
 
     $lock_name = $this->newLockName();
     $lock = PhabricatorGlobalLock::newLock($lock_name);
@@ -110,8 +108,7 @@ final class PhabricatorGlobalLockTestCase
   }
 
   public function testMultipleLocks() {
-    $conn = id(new HarbormasterScratchTable())
-      ->establishConnection('w');
+    $conn = PhabricatorGlobalLock::newConnection();
 
     PhabricatorGlobalLock::clearConnectionPool();
 
@@ -143,8 +140,88 @@ final class PhabricatorGlobalLockTestCase
       pht('Expect multiple locks on the same connection to fail.'));
   }
 
+  public function testPoolReleaseOnFailure() {
+    $conn = PhabricatorGlobalLock::newConnection();
+    $lock_name = $this->newLockName();
+
+    PhabricatorGlobalLock::clearConnectionPool();
+
+    $this->assertEqual(
+      0,
+      PhabricatorGlobalLock::getConnectionPoolSize(),
+      pht('Clear Connection Pool'));
+
+    $lock = PhabricatorGlobalLock::newLock($lock_name);
+
+    // NOTE: We're cheating here, since there's a global registry of locks
+    // for the process that we have to bypass. In the real world, this lock
+    // would have to be held by some external process. To simplify this
+    // test case, just use a raw "GET_LOCK()" call to hold the lock.
+
+    $raw_conn = PhabricatorGlobalLock::newConnection();
+    $raw_name = $lock->getName();
+
+    $row = queryfx_one(
+      $raw_conn,
+      'SELECT GET_LOCK(%s, %f)',
+      $raw_name,
+      0);
+    $this->assertTrue((bool)head($row), pht('Establish Raw Lock'));
+
+    $this->assertEqual(
+      0,
+      PhabricatorGlobalLock::getConnectionPoolSize(),
+      pht('Connection Pool with Held Lock'));
+
+    // We expect this sequence to establish a new connection, fail to acquire
+    // the lock, then put the connection in the connection pool. After the
+    // first cycle, the connection should be reused.
+
+    for ($ii = 0; $ii < 3; $ii++) {
+      $this->tryHeldLock($lock_name);
+      $this->assertEqual(
+        1,
+        PhabricatorGlobalLock::getConnectionPoolSize(),
+        pht('Connection Pool After Lock Failure'));
+    }
+
+    PhabricatorGlobalLock::clearConnectionPool();
+
+    // Now, do the same thing with an external connection. This connection
+    // should not be put into the pool! See T13627.
+
+    for ($ii = 0; $ii < 3; $ii++) {
+      $this->tryHeldLock($lock_name, $conn);
+      $this->assertEqual(
+        0,
+        PhabricatorGlobalLock::getConnectionPoolSize(),
+        pht('Connection Pool After External Lock Failure'));
+    }
+  }
+
   private function newLockName() {
     return 'testlock-'.Filesystem::readRandomCharacters(16);
   }
 
+  private function tryHeldLock(
+    $lock_name,
+    AphrontDatabaseConnection $conn = null) {
+
+    $lock = PhabricatorGlobalLock::newLock($lock_name);
+
+    if ($conn) {
+      $lock->setExternalConnection($conn);
+    }
+
+    $caught = null;
+    try {
+      $lock->lock(0);
+    } catch (PhutilLockException $ex) {
+      $caught = $ex;
+    }
+
+    $this->assertTrue($caught instanceof PhutilLockException);
+  }
+
+
 }