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

Fix a memory leak in PhabricatorGlobalLock

Summary:
We currently cache all connections in LiskDAO so we can roll back transactions when fixtured unit tests complete.

Since we establish a new connection wrapper each time we establish a global lock, this cache currently grows without bound.

Instead, pool global lock connections so we never have more than the largest number of locks we've held open at once (in PullLocalDaemon, always 1).

Another way to solve this is probably to add an "onclose" callback to `AphrontDatabaseConnection` so that it can notify any caches that it been closed. However, we currently allow a connection to be later reopened (which seeems reasonable) so we'd need a callback for that too. This is much simpler, and this use case is unusual, so I'd like to wait for more use cases before pursing a more complicated fix.

Test Plan:
Ran this in a loop:

    while (true) {
      for ($ii = 0; $ii < 100; $ii++) {
        $lock = PhabricatorGlobalLock::newLock('derp');
        $lock->lock();
        $lock->unlock();
      }
      $this->sleep(1);
    }

Previously it leaked ~100KB/sec, now has stable memory usage.

Reviewers: vrana, nh, btrahan

Reviewed By: vrana

CC: aran

Maniphest Tasks: T1636

Differential Revision: https://secure.phabricator.com/D3239
This commit is contained in:
epriestley 2012-08-10 11:28:43 -07:00
parent b1b2afce95
commit 62b06f0f5d

View file

@ -47,6 +47,8 @@ final class PhabricatorGlobalLock extends PhutilLock {
private $lockname;
private $conn;
private static $pool = array();
/* -( Constructing Locks )------------------------------------------------- */
@ -70,6 +72,12 @@ final class PhabricatorGlobalLock extends PhutilLock {
protected function doLock($wait) {
$conn = $this->conn;
if (!$conn) {
// Try to reuse a connection from the connection pool.
$conn = array_pop(self::$pool);
}
if (!$conn) {
// NOTE: Using the 'repository' database somewhat arbitrarily, mostly
// because the first client of locks is the repository daemons. We must
@ -111,6 +119,7 @@ final class PhabricatorGlobalLock extends PhutilLock {
'phabricator:'.$this->lockname);
$this->conn->close();
self::$pool[] = $this->conn;
$this->conn = null;
}