Prevent external connections from being mutated on held locks

Summary: Ref T13627. This makes the API harder to misuse: setting an external connection on a held lock isn't a meaningful operation. Prevent it.

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

Maniphest Tasks: T13627

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

src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php

@@ -311,7 +311,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject {
     $write_lock = PhabricatorRepositoryWorkingCopyVersion::getWriteLock(
       $repository_phid);
 
-    $write_lock->useSpecificConnection($locked_connection);
+    $write_lock->setExternalConnection($locked_connection);
 
     $this->logLine(
       pht(

src/infrastructure/storage/management/workflow/PhabricatorStorageManagementWorkflow.php

@@ -1229,7 +1229,7 @@ abstract class PhabricatorStorageManagementWorkflow
     // log table yet, or may need to adjust it.
 
     return PhabricatorGlobalLock::newLock('adjust', $parameters)
-      ->useSpecificConnection($api->getConn(null))
+      ->setExternalConnection($api->getConn(null))
       ->setDisableLogging(true)
       ->lock();
   }

src/infrastructure/util/PhabricatorGlobalLock.php

@@ -91,8 +91,13 @@ final class PhabricatorGlobalLock extends PhutilLock {
    * @param  AphrontDatabaseConnection
    * @return this
    */
-  public function useSpecificConnection(AphrontDatabaseConnection $conn) {
+  public function setExternalConnection(AphrontDatabaseConnection $conn) {
-    $this->conn = $conn;
+    if ($this->conn) {
+      throw new Exception(
+        pht(
+          'Lock is already held, and must be released before the '.
+          'connection may be changed.'));
+    }
     $this->externalConnection = $conn;
     return $this;
   }
@@ -118,6 +123,12 @@ final class PhabricatorGlobalLock extends PhutilLock {
   protected function doLock($wait) {
     $conn = $this->conn;
 
+    if (!$conn) {
+      if ($this->externalConnection) {
+        $conn = $this->externalConnection;
+      }
+    }
+
     if (!$conn) {
       // Try to reuse a connection from the connection pool.
       $conn = array_pop(self::$pool);

src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php

@@ -54,7 +54,7 @@ final class PhabricatorGlobalLockTestCase
 
     $lock_name = $this->newLockName();
     $lock = PhabricatorGlobalLock::newLock($lock_name);
-    $lock->useSpecificConnection($conn);
+    $lock->setExternalConnection($conn);
     $lock->lock();
 
     $this->assertEqual(
@@ -85,6 +85,30 @@ final class PhabricatorGlobalLockTestCase
     PhabricatorGlobalLock::clearConnectionPool();
   }
 
+  public function testExternalConnectionMutationScope() {
+    $conn = id(new HarbormasterScratchTable())
+      ->establishConnection('w');
+
+    $lock_name = $this->newLockName();
+    $lock = PhabricatorGlobalLock::newLock($lock_name);
+    $lock->lock();
+
+    $caught = null;
+    try {
+      $lock->setExternalConnection($conn);
+    } catch (Exception $ex) {
+      $caught = $ex;
+    } catch (Throwable $ex) {
+      $caught = $ex;
+    }
+
+    $lock->unlock();
+
+    $this->assertTrue(
+      ($caught instanceof Exception),
+      pht('Changing connection while locked is forbidden.'));
+  }
+
   private function newLockName() {
     return 'testlock-'.Filesystem::readRandomCharacters(16);
   }