1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-10 00:42:41 +01:00

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
This commit is contained in:
epriestley 2021-03-02 12:43:38 -08:00
parent 55f4a258d2
commit 15dbf6bdf0
3 changed files with 107 additions and 4 deletions

View file

@ -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',

View file

@ -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;
}

View file

@ -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);
}
}