revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-25 16:22:43 +01:00
Code Issues Releases Wiki Activity

Refuse to acquire a second GlobalLock on a connection

Browse source 
Summary:
Ref T13627. MySQL versions older than 5.7 release held locks when a new lock is acquired.

Prevent acquisition of a second lock to prevent this.

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

Maniphest Tasks: T13627

Differential Revision: https://secure.phabricator.com/D21586
This commit is contained in:
epriestley 2021-03-02 12:58:14 -08:00
parent 466013f11a
commit 2b473558c2
2 changed files with 49 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
src/infrastructure/util/PhabricatorGlobalLock.php
View file

@ -149,6 +149,20 @@ final class PhabricatorGlobalLock extends PhutilLock {
$conn = $dao->establishConnection('w', $force_new = true); $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 // 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 // the wait_timeout to an enormous value, to allow us to hold the
// connection open indefinitely (or, at least, for 24 days). // connection open indefinitely (or, at least, for 24 days).
@ -170,7 +184,7 @@ final class PhabricatorGlobalLock extends PhutilLock {
// is still good. We're done with it, so add it to the pool, just as we // is still good. We're done with it, so add it to the pool, just as we
// would if we were releasing the lock. // 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 // 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 // example, this can happen if there are a large number of webhook tasks
// in the queue. // in the queue.

34
src/infrastructure/util/__tests__/PhabricatorGlobalLockTestCase.php
View file

@ -109,6 +109,40 @@ final class PhabricatorGlobalLockTestCase
pht('Changing connection while locked is forbidden.')); 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() { private function newLockName() {
return 'testlock-'.Filesystem::readRandomCharacters(16); return 'testlock-'.Filesystem::readRandomCharacters(16);
} }