Page MenuHomePhabricator
Files F15453899

D21586.id.diff
No OneTemporary
Actions

D21586.id.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
@@ -149,6 +149,20 @@
$conn = $dao->establishConnection('w', $force_new = true);
}
+ // See T13627. We must never hold more than one lock per connection, so
+ // make sure this connection has no existing locks. (Normally, we should
+ // only be able to get here if callers explicitly provide the same external
+ // connection to multiple locks.)
+
+ if ($conn->isHoldingAnyLock()) {
+ throw new Exception(
+ pht(
+ 'Unable to establish lock on connection: this connection is '.
+ 'already holding a lock. Acquiring a second lock on the same '.
+ 'connection would release the first lock in MySQL versions '.
+ 'older than 5.7.'));
+ }
+
// NOTE: Since MySQL will disconnect us if we're idle for too long, we set
// the wait_timeout to an enormous value, to allow us to hold the
// connection open indefinitely (or, at least, for 24 days).
@@ -170,7 +184,7 @@
// is still good. We're done with it, so add it to the pool, just as we
// would if we were releasing the lock.
- // If we don't do this, we may establish a huge number of connections
+ // If we don't do this, we may establish a huge number of connections
// very rapidly if many workers try to acquire a lock at once. For
// example, this can happen if there are a large number of webhook tasks
// in the queue.
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
@@ -109,6 +109,40 @@
pht('Changing connection while locked is forbidden.'));
}
+ public function testMultipleLocks() {
+ $conn = id(new HarbormasterScratchTable())
+ ->establishConnection('w');
+
+ PhabricatorGlobalLock::clearConnectionPool();
+
+ $lock_name_a = $this->newLockName();
+ $lock_name_b = $this->newLockName();
+
+ $lock_a = PhabricatorGlobalLock::newLock($lock_name_a);
+ $lock_a->setExternalConnection($conn);
+
+ $lock_b = PhabricatorGlobalLock::newLock($lock_name_b);
+ $lock_b->setExternalConnection($conn);
+
+ $lock_a->lock();
+
+ $caught = null;
+ try {
+ $lock_b->lock();
+ } catch (Exception $ex) {
+ $caught = $ex;
+ } catch (Throwable $ex) {
+ $caught = $ex;
+ }
+
+ // See T13627. The lock infrastructure must forbid this because it does
+ // not work in versions of MySQL older than 5.7.
+
+ $this->assertTrue(
+ ($caught instanceof Exception),
+ pht('Expect multiple locks on the same connection to fail.'));
+ }
+
private function newLockName() {
return 'testlock-'.Filesystem::readRandomCharacters(16);
}

File Metadata

Mime Type
text/plain
Expires
Sun, Mar 30, 2:57 PM (5 d, 12 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7684704
Default Alt Text
D21586.id.diff (3 KB)

Event Timeline

Phabricator · Built by Phacility