From f148dcabc45c2c793f28518aa2deec6d51826546 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Wed, 5 Sep 2018 22:16:23 +0200 Subject: [PATCH 1/2] CacheLockDriverTest Fixings - Changing test behaviour --- .../Core/Lock/ArrayCacheLockDriverTest.php | 14 +------ tests/src/Core/Lock/LockTest.php | 37 +++++++++++++------ .../Core/Lock/MemcacheCacheLockDriverTest.php | 15 +------- .../Lock/MemcachedCacheLockDriverTest.php | 15 +------- .../Core/Lock/RedisCacheLockDriverTest.php | 15 +------- .../src/Core/Lock/SemaphoreLockDriverTest.php | 15 +------- 6 files changed, 31 insertions(+), 80 deletions(-) diff --git a/tests/src/Core/Lock/ArrayCacheLockDriverTest.php b/tests/src/Core/Lock/ArrayCacheLockDriverTest.php index dc044f5c5c..671341718b 100644 --- a/tests/src/Core/Lock/ArrayCacheLockDriverTest.php +++ b/tests/src/Core/Lock/ArrayCacheLockDriverTest.php @@ -8,21 +8,9 @@ use Friendica\Core\Lock\CacheLockDriver; class ArrayCacheLockDriverTest extends LockTest { - /** - * @var \Friendica\Core\Cache\IMemoryCacheDriver - */ - private $cache; - protected function getInstance() { - $this->cache = new ArrayCache(); - return new CacheLockDriver($this->cache); - } - - public function tearDown() - { - $this->cache->clear(); - parent::tearDown(); + return new CacheLockDriver(new ArrayCache()); } public function testLockTTL() diff --git a/tests/src/Core/Lock/LockTest.php b/tests/src/Core/Lock/LockTest.php index 9698f0bdea..a7bbea265f 100644 --- a/tests/src/Core/Lock/LockTest.php +++ b/tests/src/Core/Lock/LockTest.php @@ -19,6 +19,7 @@ abstract class LockTest extends DatabaseTest { parent::setUp(); $this->instance = $this->getInstance(); + $this->instance->releaseAll(); // Reusable App object $this->app = BaseObject::getApp(); @@ -31,11 +32,18 @@ abstract class LockTest extends DatabaseTest Config::set('system', 'theme', 'system_theme'); } + protected function tearDown() + { + parent::tearDown(); + $this->instance->releaseAll(); + } + /** * @small */ public function testLock() { - $this->instance->acquireLock('foo', 1); + $this->assertFalse($this->instance->isLocked('foo')); + $this->assertTrue($this->instance->acquireLock('foo', 1)); $this->assertTrue($this->instance->isLocked('foo')); $this->assertFalse($this->instance->isLocked('bar')); } @@ -44,7 +52,8 @@ abstract class LockTest extends DatabaseTest * @small */ public function testDoubleLock() { - $this->instance->acquireLock('foo', 1); + $this->assertFalse($this->instance->isLocked('foo')); + $this->assertTrue($this->instance->acquireLock('foo', 1)); $this->assertTrue($this->instance->isLocked('foo')); // We already locked it $this->assertTrue($this->instance->acquireLock('foo', 1)); @@ -54,7 +63,8 @@ abstract class LockTest extends DatabaseTest * @small */ public function testReleaseLock() { - $this->instance->acquireLock('foo', 1); + $this->assertFalse($this->instance->isLocked('foo')); + $this->assertTrue($this->instance->acquireLock('foo', 1)); $this->assertTrue($this->instance->isLocked('foo')); $this->instance->releaseLock('foo'); $this->assertFalse($this->instance->isLocked('foo')); @@ -64,9 +74,9 @@ abstract class LockTest extends DatabaseTest * @small */ public function testReleaseAll() { - $this->instance->acquireLock('foo', 1); - $this->instance->acquireLock('bar', 1); - $this->instance->acquireLock('nice', 1); + $this->assertTrue($this->instance->acquireLock('foo', 1)); + $this->assertTrue($this->instance->acquireLock('bar', 1)); + $this->assertTrue($this->instance->acquireLock('nice', 1)); $this->assertTrue($this->instance->isLocked('foo')); $this->assertTrue($this->instance->isLocked('bar')); @@ -83,9 +93,12 @@ abstract class LockTest extends DatabaseTest * @small */ public function testReleaseAfterUnlock() { - $this->instance->acquireLock('foo', 1); - $this->instance->acquireLock('bar', 1); - $this->instance->acquireLock('nice', 1); + $this->assertFalse($this->instance->isLocked('foo')); + $this->assertFalse($this->instance->isLocked('bar')); + $this->assertFalse($this->instance->isLocked('nice')); + $this->assertTrue($this->instance->acquireLock('foo', 1)); + $this->assertTrue($this->instance->acquireLock('bar', 1)); + $this->assertTrue($this->instance->acquireLock('nice', 1)); $this->instance->releaseLock('foo'); @@ -103,10 +116,12 @@ abstract class LockTest extends DatabaseTest * @medium */ function testLockTTL() { + $this->assertFalse($this->instance->isLocked('foo')); + $this->assertFalse($this->instance->isLocked('bar')); // TODO [nupplaphil] - Because of the Datetime-Utils for the database, we have to wait a FULL second between the checks to invalidate the db-locks/cache - $this->instance->acquireLock('foo', 1, 1); - $this->instance->acquireLock('bar', 1, 3); + $this->assertTrue($this->instance->acquireLock('foo', 2, 1)); + $this->assertTrue($this->instance->acquireLock('bar', 2, 3)); $this->assertTrue($this->instance->isLocked('foo')); $this->assertTrue($this->instance->isLocked('bar')); diff --git a/tests/src/Core/Lock/MemcacheCacheLockDriverTest.php b/tests/src/Core/Lock/MemcacheCacheLockDriverTest.php index c54cf9c599..ca9b9b4639 100644 --- a/tests/src/Core/Lock/MemcacheCacheLockDriverTest.php +++ b/tests/src/Core/Lock/MemcacheCacheLockDriverTest.php @@ -3,7 +3,6 @@ namespace Friendica\Test\src\Core\Lock; - use Friendica\Core\Cache\CacheDriverFactory; use Friendica\Core\Lock\CacheLockDriver; @@ -12,20 +11,8 @@ use Friendica\Core\Lock\CacheLockDriver; */ class MemcacheCacheLockDriverTest extends LockTest { - /** - * @var \Friendica\Core\Cache\IMemoryCacheDriver - */ - private $cache; - protected function getInstance() { - $this->cache = CacheDriverFactory::create('memcache'); - return new CacheLockDriver($this->cache); - } - - public function tearDown() - { - $this->cache->clear(); - parent::tearDown(); + return new CacheLockDriver(CacheDriverFactory::create('memcache')); } } diff --git a/tests/src/Core/Lock/MemcachedCacheLockDriverTest.php b/tests/src/Core/Lock/MemcachedCacheLockDriverTest.php index 27886261b2..71311d5baa 100644 --- a/tests/src/Core/Lock/MemcachedCacheLockDriverTest.php +++ b/tests/src/Core/Lock/MemcachedCacheLockDriverTest.php @@ -3,7 +3,6 @@ namespace Friendica\Test\src\Core\Lock; - use Friendica\Core\Cache\CacheDriverFactory; use Friendica\Core\Lock\CacheLockDriver; @@ -12,20 +11,8 @@ use Friendica\Core\Lock\CacheLockDriver; */ class MemcachedCacheLockDriverTest extends LockTest { - /** - * @var \Friendica\Core\Cache\IMemoryCacheDriver - */ - private $cache; - protected function getInstance() { - $this->cache = CacheDriverFactory::create('memcached'); - return new CacheLockDriver($this->cache); - } - - public function tearDown() - { - $this->cache->clear(); - parent::tearDown(); + return new CacheLockDriver(CacheDriverFactory::create('memcached')); } } diff --git a/tests/src/Core/Lock/RedisCacheLockDriverTest.php b/tests/src/Core/Lock/RedisCacheLockDriverTest.php index eaafbf4e44..43cedb9cbc 100644 --- a/tests/src/Core/Lock/RedisCacheLockDriverTest.php +++ b/tests/src/Core/Lock/RedisCacheLockDriverTest.php @@ -3,7 +3,6 @@ namespace Friendica\Test\src\Core\Lock; - use Friendica\Core\Cache\CacheDriverFactory; use Friendica\Core\Lock\CacheLockDriver; @@ -12,21 +11,9 @@ use Friendica\Core\Lock\CacheLockDriver; */ class RedisCacheLockDriverTest extends LockTest { - /** - * @var \Friendica\Core\Cache\IMemoryCacheDriver - */ - private $cache; - protected function getInstance() { - $this->cache = CacheDriverFactory::create('redis'); - return new CacheLockDriver($this->cache); + return new CacheLockDriver(CacheDriverFactory::create('redis')); } - - public function tearDown() - { - $this->cache->clear(); - parent::tearDown(); - } } diff --git a/tests/src/Core/Lock/SemaphoreLockDriverTest.php b/tests/src/Core/Lock/SemaphoreLockDriverTest.php index cd4b915733..39f0763fb1 100644 --- a/tests/src/Core/Lock/SemaphoreLockDriverTest.php +++ b/tests/src/Core/Lock/SemaphoreLockDriverTest.php @@ -2,26 +2,13 @@ namespace Friendica\Test\src\Core\Lock; - use Friendica\Core\Lock\SemaphoreLockDriver; class SemaphoreLockDriverTest extends LockTest { - /** - * @var \Friendica\Core\Lock\SemaphoreLockDriver - */ - private $semaphoreLockDriver; - protected function getInstance() { - $this->semaphoreLockDriver = new SemaphoreLockDriver(); - return $this->semaphoreLockDriver; - } - - public function tearDown() - { - $this->semaphoreLockDriver->releaseAll(); - parent::tearDown(); + return new SemaphoreLockDriver(); } function testLockTTL() From bd2b3b1ef5e066f927bf8ae95954dd4569319bca Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Thu, 6 Sep 2018 08:11:18 +0200 Subject: [PATCH 2/2] LockDriverFixings - release Locks in case of failures - adding some cache tests --- src/Core/Cache/ArrayCache.php | 5 +++++ src/Core/Worker.php | 2 ++ src/Model/Item.php | 1 + tests/src/Core/Cache/CacheTest.php | 24 +++++++++++++++++++----- 4 files changed, 27 insertions(+), 5 deletions(-) diff --git a/src/Core/Cache/ArrayCache.php b/src/Core/Cache/ArrayCache.php index b198287148..d1302c1d6e 100644 --- a/src/Core/Cache/ArrayCache.php +++ b/src/Core/Cache/ArrayCache.php @@ -53,6 +53,11 @@ class ArrayCache extends AbstractCacheDriver implements IMemoryCacheDriver */ public function clear($outdated = true) { + // Array doesn't support TTL so just don't delete something + if ($outdated) { + return true; + } + $this->cachedData = []; return true; } diff --git a/src/Core/Worker.php b/src/Core/Worker.php index 9dd973728d..3400f00ae1 100644 --- a/src/Core/Worker.php +++ b/src/Core/Worker.php @@ -117,12 +117,14 @@ class Worker // Count active workers and compare them with a maximum value that depends on the load if (self::tooMuchWorkers()) { logger('Active worker limit reached, quitting.', LOGGER_DEBUG); + Lock::release('worker'); return; } // Check free memory if ($a->min_memory_reached()) { logger('Memory limit reached, quitting.', LOGGER_DEBUG); + Lock::release('worker'); return; } Lock::release('worker'); diff --git a/src/Model/Item.php b/src/Model/Item.php index d235f0a7f9..10c705a995 100644 --- a/src/Model/Item.php +++ b/src/Model/Item.php @@ -1936,6 +1936,7 @@ class Item extends BaseObject } else { // This shouldn't happen. logger('Could not insert activity for URI ' . $item['uri'] . ' - should not happen'); + Lock::release('item_insert_activity'); return false; } if ($locked) { diff --git a/tests/src/Core/Cache/CacheTest.php b/tests/src/Core/Cache/CacheTest.php index 105142f0b7..5c56c2072f 100644 --- a/tests/src/Core/Cache/CacheTest.php +++ b/tests/src/Core/Cache/CacheTest.php @@ -81,19 +81,33 @@ abstract class CacheTest extends DatabaseTest '3_value1' => $this->instance->get('3_value1'), ]); - $this->assertTrue($this->instance->clear(false)); + $this->assertTrue($this->instance->clear()); $this->assertEquals([ - '1_value1' => null, - '1_value2' => null, - '2_value1' => null, - '3_value1' => null, + '1_value1' => 'ipsum lorum1', + '1_value2' => 'ipsum lorum2', + '2_value1' => 'ipsum lorum3', + '3_value1' => 'ipsum lorum4', ], [ '1_value1' => $this->instance->get('1_value1'), '1_value2' => $this->instance->get('1_value2'), '2_value1' => $this->instance->get('2_value1'), '3_value1' => $this->instance->get('3_value1'), ]); + + $this->assertTrue($this->instance->clear(false)); + + $this->assertEquals([ + '1_value1' => null, + '1_value2' => null, + '2_value3' => null, + '3_value4' => null, + ], [ + '1_value1' => $this->instance->get('1_value1'), + '1_value2' => $this->instance->get('1_value2'), + '2_value3' => $this->instance->get('2_value3'), + '3_value4' => $this->instance->get('3_value4'), + ]); } /**