Page MenuHomePhabricator
Files F15333956

D21585.diff
No OneTemporary
Actions

D21585.diff
View Options

diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php
--- a/src/infrastructure/util/PhabricatorGlobalLock.php
+++ b/src/infrastructure/util/PhabricatorGlobalLock.php
@@ -118,6 +118,28 @@
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 @@
}
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 @@
// 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));
diff --git a/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php b/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php
--- a/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php
+++ b/src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php
@@ -37,8 +37,7 @@
}
public function testConnectionPoolWithSpecificConnection() {
- $conn = id(new HarbormasterScratchTable())
- ->establishConnection('w');
+ $conn = PhabricatorGlobalLock::newConnection();
PhabricatorGlobalLock::clearConnectionPool();
@@ -86,8 +85,7 @@
}
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 @@
}
public function testMultipleLocks() {
- $conn = id(new HarbormasterScratchTable())
- ->establishConnection('w');
+ $conn = PhabricatorGlobalLock::newConnection();
PhabricatorGlobalLock::clearConnectionPool();
@@ -143,8 +140,88 @@
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);
+ }
+
+
}

File Metadata

Mime Type
text/plain
Expires
Sun, Mar 9, 5:54 AM (1 w, 6 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7384842
Default Alt Text
D21585.diff (6 KB)

Event Timeline

Phabricator · Built by Phacility