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