From aa2d89f1d4c57ed86726b92fa6d75a6be027dbaa Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Tue, 2 Mar 2021 12:43:38 -0800
Subject: [PATCH] (stable) When a GlobalLock with an external connection is
 released, don't return it to the pool

Summary:
Ref T13627. Currently, global locks always return connections (even external connections) to the connection pool when unlocked.

This code is obviously buggy: `isExternalConnection` is set to false immediately before it is tested. This bug has existed since this code was introduced, in D15792.

  - Instead of storing a flag, store the actual connection.
  - Don't clear it when unlocking.
  - Don't return external connections to the pool.

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

Maniphest Tasks: T13627

Differential Revision: https://secure.phabricator.com/D21583
---
 src/__phutil_library_map__.php                |  2 +
 .../util/PhabricatorGlobalLock.php            | 17 +++-
 .../PhabricatorGlobalLockTestCase.php         | 92 +++++++++++++++++++
 3 files changed, 107 insertions(+), 4 deletions(-)
 create mode 100644 src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
index 09b04e8a16..136ba41682 100644
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -3548,6 +3548,7 @@ phutil_register_library_map(array(
     'PhabricatorGitGraphStream' => 'applications/repository/daemon/PhabricatorGitGraphStream.php',
     'PhabricatorGitHubAuthProvider' => 'applications/auth/provider/PhabricatorGitHubAuthProvider.php',
     'PhabricatorGlobalLock' => 'infrastructure/util/PhabricatorGlobalLock.php',
+    'PhabricatorGlobalLockTestCase' => 'infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php',
     'PhabricatorGlobalUploadTargetView' => 'applications/files/view/PhabricatorGlobalUploadTargetView.php',
     'PhabricatorGoogleAuthProvider' => 'applications/auth/provider/PhabricatorGoogleAuthProvider.php',
     'PhabricatorGuidanceContext' => 'applications/guides/guidance/PhabricatorGuidanceContext.php',
@@ -10093,6 +10094,7 @@ phutil_register_library_map(array(
     'PhabricatorGitGraphStream' => 'PhabricatorRepositoryGraphStream',
     'PhabricatorGitHubAuthProvider' => 'PhabricatorOAuth2AuthProvider',
     'PhabricatorGlobalLock' => 'PhutilLock',
+    'PhabricatorGlobalLockTestCase' => 'PhabricatorTestCase',
     'PhabricatorGlobalUploadTargetView' => 'AphrontView',
     'PhabricatorGoogleAuthProvider' => 'PhabricatorOAuth2AuthProvider',
     'PhabricatorGuidanceContext' => 'Phobject',
diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php
index cc0641b106..cf380c3726 100644
--- a/src/infrastructure/util/PhabricatorGlobalLock.php
+++ b/src/infrastructure/util/PhabricatorGlobalLock.php
@@ -30,7 +30,7 @@ final class PhabricatorGlobalLock extends PhutilLock {
 
   private $parameters;
   private $conn;
-  private $isExternalConnection = false;
+  private $externalConnection;
   private $log;
   private $disableLogging;
 
@@ -93,7 +93,7 @@ final class PhabricatorGlobalLock extends PhutilLock {
    */
   public function useSpecificConnection(AphrontDatabaseConnection $conn) {
     $this->conn = $conn;
-    $this->isExternalConnection = true;
+    $this->externalConnection = $conn;
     return $this;
   }
 
@@ -103,6 +103,16 @@ final class PhabricatorGlobalLock extends PhutilLock {
   }
 
 
+/* -(  Connection Pool  )---------------------------------------------------- */
+
+  public static function getConnectionPoolSize() {
+    return count(self::$pool);
+  }
+
+  public static function clearConnectionPool() {
+    self::$pool = array();
+  }
+
 /* -(  Implementation  )----------------------------------------------------- */
 
   protected function doLock($wait) {
@@ -201,9 +211,8 @@ final class PhabricatorGlobalLock extends PhutilLock {
     }
 
     $this->conn = null;
-    $this->isExternalConnection = false;
 
-    if (!$this->isExternalConnection) {
+    if (!$this->externalConnection) {
       $conn->close();
       self::$pool[] = $conn;
     }
diff --git a/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php b/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php
new file mode 100644
index 0000000000..4e24cdfed4
--- /dev/null
+++ b/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php
@@ -0,0 +1,92 @@
+<?php
+
+final class PhabricatorGlobalLockTestCase
+  extends PhabricatorTestCase {
+
+  protected function getPhabricatorTestCaseConfiguration() {
+    return array(
+      self::PHABRICATOR_TESTCONFIG_BUILD_STORAGE_FIXTURES => true,
+    );
+  }
+
+  public function testConnectionPoolWithDefaultConnection() {
+    PhabricatorGlobalLock::clearConnectionPool();
+
+    $this->assertEqual(
+      0,
+      PhabricatorGlobalLock::getConnectionPoolSize(),
+      pht('Clear Connection Pool'));
+
+    $lock_name = $this->newLockName();
+    $lock = PhabricatorGlobalLock::newLock($lock_name);
+    $lock->lock();
+
+    $this->assertEqual(
+      0,
+      PhabricatorGlobalLock::getConnectionPoolSize(),
+      pht('Connection Pool With Lock'));
+
+    $lock->unlock();
+
+    $this->assertEqual(
+      1,
+      PhabricatorGlobalLock::getConnectionPoolSize(),
+      pht('Connection Pool With Lock Released'));
+
+    PhabricatorGlobalLock::clearConnectionPool();
+  }
+
+  public function testConnectionPoolWithSpecificConnection() {
+    $conn = id(new HarbormasterScratchTable())
+      ->establishConnection('w');
+
+    PhabricatorGlobalLock::clearConnectionPool();
+
+    $this->assertEqual(
+      0,
+      PhabricatorGlobalLock::getConnectionPoolSize(),
+      pht('Clear Connection Pool'));
+
+    $this->assertEqual(
+      false,
+      $conn->isHoldingAnyLock(),
+      pht('Specific Connection, No Lock'));
+
+    $lock_name = $this->newLockName();
+    $lock = PhabricatorGlobalLock::newLock($lock_name);
+    $lock->useSpecificConnection($conn);
+    $lock->lock();
+
+    $this->assertEqual(
+      0,
+      PhabricatorGlobalLock::getConnectionPoolSize(),
+      pht('Connection Pool + Specific, With Lock'));
+
+    $this->assertEqual(
+      true,
+      $conn->isHoldingAnyLock(),
+      pht('Specific Connection, Holding Lock'));
+
+    $lock->unlock();
+
+    // The specific connection provided should NOT be returned to the
+    // connection pool.
+
+    $this->assertEqual(
+      0,
+      PhabricatorGlobalLock::getConnectionPoolSize(),
+      pht('Connection Pool + Specific, With Lock Released'));
+
+    $this->assertEqual(
+      false,
+      $conn->isHoldingAnyLock(),
+      pht('Specific Connection, No Lock'));
+
+    PhabricatorGlobalLock::clearConnectionPool();
+  }
+
+  private function newLockName() {
+    return 'testlock-'.Filesystem::readRandomCharacters(16);
+  }
+
+}