put rexpiring delay logic in Lock class (#15874)

* Only use one reader db instance per LogAggregator.

* use re-expire logic in Lock itself

* tweak

* Remove unneeded var

* fix test

4 files changed, 80 insertions, 13 deletions

diff --git a/core/Concurrency/Lock.php b/core/Concurrency/Lock.php
index f26f589699..3063078865 100644
--- a/core/Concurrency/Lock.php
+++ b/core/Concurrency/Lock.php
@@ -8,11 +8,14 @@
  */
 namespace Piwik\Concurrency;
 
+use Piwik\ArchiveProcessor\ArchivingStatus;
 use Piwik\Common;
+use Piwik\Date;
 
 class Lock
 {
     const MAX_KEY_LEN = 70;
+    const DEFAULT_TTL = 60;
 
     /**
      * @var LockBackend
@@ -24,18 +27,28 @@ class Lock
     private $lockKey   = null;
     private $lockValue = null;
     private $defaultTtl = null;
+    private $lastExpireTime = null;
 
     public function __construct(LockBackend $backend, $lockKeyStart, $defaultTtl = null)
     {
         $this->backend = $backend;
         $this->lockKeyStart = $lockKeyStart;
         $this->lockKey = $this->lockKeyStart;
-        $this->defaultTtl = $defaultTtl;
+        $this->defaultTtl = $defaultTtl ?: self::DEFAULT_TTL;
     }
 
     public function reexpireLock()
     {
-        $this->expireLock($this->defaultTtl);
+        $timeBetweenReexpires = $this->defaultTtl - ($this->defaultTtl / 4);
+
+        $now = Date::getNowTimestamp();
+        if (!empty($this->lastExpireTime) &&
+            $now <= $this->lastExpireTime + $timeBetweenReexpires
+        ) {
+            return false;
+        }
+
+        return $this->expireLock($this->defaultTtl);
     }
 
     public function getNumberOfAcquiredLocks()
@@ -81,6 +94,8 @@ class Lock
 
         if ($locked) {
             $this->lockValue = $lockValue;
+            $this->ttlUsed = $ttlInSeconds;
+            $this->lastExpireTime = Date::getNowTimestamp();
         }
 
         return $locked;
@@ -125,6 +140,8 @@ class Lock
                     return false;
                 }
 
+                $this->lastExpireTime = Date::getNowTimestamp();
+
                 return true;
             } else {
                 Common::printDebug('Lock is not acquired, cannot update expiration.');
diff --git a/core/DataAccess/ArchivingDbAdapter.php b/core/DataAccess/ArchivingDbAdapter.php
index 83d24fc4a7..7c0b5ccf49 100644
--- a/core/DataAccess/ArchivingDbAdapter.php
+++ b/core/DataAccess/ArchivingDbAdapter.php
@@ -31,11 +31,6 @@ class ArchivingDbAdapter
      */
     private $logger;
 
-    /**
-     * @var int
-     */
-    private $lastReexpireTime = null;
-
     public function __construct($wrapped, Lock $archivingLock = null, LoggerInterface $logger = null)
     {
         $this->wrapped = $wrapped;
@@ -107,11 +102,7 @@ class ArchivingDbAdapter
     private function reexpireLock()
     {
         if ($this->archivingLock) {
-            $timeBetweenReexpires = ArchivingStatus::DEFAULT_ARCHIVING_TTL / 4;
-            if ($this->lastReexpireTime + $timeBetweenReexpires < time()) {
-                $this->archivingLock->reexpireLock();
-                $this->lastReexpireTime = time();
-            }
+            $this->archivingLock->reexpireLock();
         }
     }
 }
\ No newline at end of file
diff --git a/tests/PHPUnit/Integration/Concurrency/LockTest.php b/tests/PHPUnit/Integration/Concurrency/LockTest.php
index cb1345c2f2..d0509a03ca 100644
--- a/tests/PHPUnit/Integration/Concurrency/LockTest.php
+++ b/tests/PHPUnit/Integration/Concurrency/LockTest.php
@@ -9,8 +9,11 @@
 namespace Piwik\Tests\Integration\Concurrency;
 
 
+use Piwik\Common;
 use Piwik\Concurrency\Lock;
 use Piwik\Concurrency\LockBackend\MySqlLockBackend;
+use Piwik\Date;
+use Piwik\Db;
 use Piwik\Tests\Framework\TestCase\IntegrationTestCase;
 
 /**
@@ -128,6 +131,54 @@ class LockTest extends IntegrationTestCase
         $this->assertSame(array('TestLock0', 'TestLock4', 'TestLock5'), $locks);
     }
 
+    public function test_rexpire_onlyRexpiresWhenCloseToOriginalExpirationTime()
+    {
+        Date::$now = strtotime('2015-03-04 03:04:05');
+
+        $this->lock->acquireLock(0);
+
+        $expireTime = $this->getLockExpirationTime();
+
+        sleep(1);
+        $this->lock->reexpireLock();
+        $newExpireTime = $this->getLockExpirationTime();
+        $this->assertEquals($expireTime, $newExpireTime);
+
+        // 30s after initial, no update
+        Date::$now = strtotime('2015-03-04 03:04:35');
+
+        sleep(1);
+        $this->lock->reexpireLock();
+        $newExpireTime = $this->getLockExpirationTime();
+        $this->assertEquals($expireTime, $newExpireTime);
+
+        // 50s after initial, update
+        Date::$now = strtotime('2015-03-04 03:04:55');
+
+        sleep(1);
+        $this->lock->reexpireLock();
+        $newExpireTime = $this->getLockExpirationTime();
+        $this->assertNotEquals($expireTime, $newExpireTime);
+
+        $expireTime = $newExpireTime;
+
+        // 60s after initial, no update
+        Date::$now = strtotime('2015-03-04 03:05:05');
+
+        sleep(1);
+        $this->lock->reexpireLock();
+        $newExpireTime = $this->getLockExpirationTime();
+        $this->assertEquals($expireTime, $newExpireTime);
+
+        // 1m 50s after initial, update
+        Date::$now = strtotime('2015-03-04 03:05:55');
+
+        sleep(1);
+        $this->lock->reexpireLock();
+        $newExpireTime = $this->getLockExpirationTime();
+        $this->assertNotEquals($expireTime, $newExpireTime);
+    }
+
     private function assertNumberOfLocksEquals($numExpectedLocks)
     {
         $this->assertSame($numExpectedLocks, $this->lock->getNumberOfAcquiredLocks());
@@ -138,4 +189,9 @@ class LockTest extends IntegrationTestCase
         return new Lock($mysql, 'TestLock');
     }
 
+    private function getLockExpirationTime()
+    {
+        return Db::fetchOne("SELECT expiry_time FROM `" . Common::prefixTable('locks') . "`");
+    }
+
 }
diff --git a/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php b/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php
index 817595270b..e65f10fcac 100644
--- a/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php
+++ b/tests/PHPUnit/Integration/DataAccess/LogAggregatorTest.php
@@ -159,8 +159,9 @@ class LogAggregatorTest extends IntegrationTestCase
         $t->setUrl('http://example.com/here/we/go');
         $t->doTrackPageView('here we go');
 
-        $params = new Parameters(new Site(self::$fixture->idSite), Period\Factory::build('day', self::$fixture->dateTime), new Segment('', [self::$fixture->idSite]));
+        Date::$now = strtotime('2015-03-04 00:08:04');
 
+        $params = new Parameters(new Site(self::$fixture->idSite), Period\Factory::build('day', self::$fixture->dateTime), new Segment('', [self::$fixture->idSite]));
         $archiveStatus = StaticContainer::get(ArchivingStatus::class);
         $archiveStatus->archiveStarted($params);
 
@@ -170,6 +171,8 @@ class LogAggregatorTest extends IntegrationTestCase
 
         sleep(1);
 
+        Date::$now = strtotime('2015-03-04 10:08:04');
+
         $this->logAggregator->queryVisitsByDimension(['visit_total_time']);
 
         $locks = $this->getAllLocks();