From 1ffe0cfd818ad48f7c227915d3abf68d26d8a06c Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Sat, 7 Jul 2018 16:15:03 +0200 Subject: [PATCH] Added Lock Unittests & Bugfixings Added Redis Lock Unittests Added Memcached Lock Unittests Fixed a bug in dba Fixed a bug in RedisLock --- .travis.yml | 4 ++ include/dba.php | 2 +- src/Core/Cache/AbstractCacheDriver.php | 1 + src/Core/Cache/RedisCacheDriver.php | 31 +++++-------- ...rTest.php => ArrayCacheLockDriverTest.php} | 4 +- .../src/Core/Lock/DatabaseLockDriverTest.php | 45 +------------------ tests/src/Core/Lock/LockTest.php | 15 +++++-- .../Lock/MemcachedCacheLockDriverTest.php | 40 +++++++++++++++++ .../Core/Lock/RedisCacheLockDriverTest.php | 40 +++++++++++++++++ .../src/Core/Lock/SemaphoreLockDriverTest.php | 2 +- 10 files changed, 114 insertions(+), 70 deletions(-) rename tests/src/Core/Lock/{CacheLockDriverTest.php => ArrayCacheLockDriverTest.php} (89%) create mode 100644 tests/src/Core/Lock/MemcachedCacheLockDriverTest.php create mode 100644 tests/src/Core/Lock/RedisCacheLockDriverTest.php diff --git a/.travis.yml b/.travis.yml index 78c29817db..6e7ac1c2e0 100644 --- a/.travis.yml +++ b/.travis.yml @@ -9,6 +9,8 @@ php: services: - mysql + - redis-server + - memcached env: - MYSQL_HOST=localhost MYSQL_PORT=3306 MYSQL_USERNAME=travis MYSQL_PASSWORD= MYSQL_DATABASE=test @@ -17,3 +19,5 @@ install: before_script: - mysql -e 'CREATE DATABASE IF NOT EXISTS test;' - mysql -utravis test < database.sql + - echo "extension=redis.so" >> ~/.phpenv/versions/$(phpenv version-name)/etc/php.ini + - echo "extension=memcached.so" >> ~/.phpenv/versions/$(phpenv version-name)/etc/php.ini diff --git a/include/dba.php b/include/dba.php index 9d828f8b44..061f5399c7 100644 --- a/include/dba.php +++ b/include/dba.php @@ -1109,7 +1109,7 @@ class dba { // Split the SQL queries in chunks of 100 values // We do the $i stuff here to make the code better readable - $i = $counter[$key_table][$key_condition]; + $i = isset($counter[$key_table][$key_condition]) ? $counter[$key_table][$key_condition] : 0; if (isset($compacted[$key_table][$key_condition][$i]) && count($compacted[$key_table][$key_condition][$i]) > 100) { ++$i; } diff --git a/src/Core/Cache/AbstractCacheDriver.php b/src/Core/Cache/AbstractCacheDriver.php index 417accdce7..15b822dc3b 100644 --- a/src/Core/Cache/AbstractCacheDriver.php +++ b/src/Core/Cache/AbstractCacheDriver.php @@ -18,6 +18,7 @@ abstract class AbstractCacheDriver extends BaseObject * @return string The cache key used for the cache */ protected function getCacheKey($key) { + // We fetch with the hostname as key to avoid problems with other applications return self::getApp()->get_hostname() . ":" . $key; } } diff --git a/src/Core/Cache/RedisCacheDriver.php b/src/Core/Cache/RedisCacheDriver.php index 67df8e8fee..c0006822d5 100644 --- a/src/Core/Cache/RedisCacheDriver.php +++ b/src/Core/Cache/RedisCacheDriver.php @@ -35,15 +35,12 @@ class RedisCacheDriver extends AbstractCacheDriver implements IMemoryCacheDriver $return = null; $cachekey = $this->getCacheKey($key); - // We fetch with the hostname as key to avoid problems with other applications $cached = $this->redis->get($cachekey); - - // @see http://php.net/manual/en/redis.get.php#84275 - if (is_bool($cached) || is_double($cached) || is_long($cached)) { - return $return; + if ($cached === false && !$this->redis->exists($cachekey)) { + return null; } - $value = @unserialize($cached); + $value = json_decode($cached); // Only return a value if the serialized value is valid. // We also check if the db entry is a serialized @@ -59,17 +56,18 @@ class RedisCacheDriver extends AbstractCacheDriver implements IMemoryCacheDriver { $cachekey = $this->getCacheKey($key); - // We store with the hostname as key to avoid problems with other applications + $cached = json_encode($value); + if ($ttl > 0) { return $this->redis->setex( $cachekey, time() + $ttl, - serialize($value) + $cached ); } else { return $this->redis->set( $cachekey, - serialize($value) + $cached ); } } @@ -81,7 +79,7 @@ class RedisCacheDriver extends AbstractCacheDriver implements IMemoryCacheDriver public function clear() { - return true; + return $this->redis->flushAll(); } @@ -91,10 +89,7 @@ class RedisCacheDriver extends AbstractCacheDriver implements IMemoryCacheDriver public function add($key, $value, $ttl = Cache::FIVE_MINUTES) { $cachekey = $this->getCacheKey($key); - - if (!is_int($value)) { - $value = serialize($value); - } + $cached = json_encode($value); return $this->redis->setnx($cachekey, $value); } @@ -106,16 +101,14 @@ class RedisCacheDriver extends AbstractCacheDriver implements IMemoryCacheDriver { $cachekey = $this->getCacheKey($key); - if (!is_int($newValue)) { - $newValue = serialize($newValue); - } + $newCached = json_encode($newValue); $this->redis->watch($cachekey); // If the old value isn't what we expected, somebody else changed the key meanwhile - if ($this->get($cachekey) === $oldValue) { + if ($this->get($key) === $oldValue) { if ($ttl > 0) { $result = $this->redis->multi() - ->setex($cachekey, $ttl, $newValue) + ->setex($cachekey, $ttl, $newCached) ->exec(); } else { $result = $this->redis->multi() diff --git a/tests/src/Core/Lock/CacheLockDriverTest.php b/tests/src/Core/Lock/ArrayCacheLockDriverTest.php similarity index 89% rename from tests/src/Core/Lock/CacheLockDriverTest.php rename to tests/src/Core/Lock/ArrayCacheLockDriverTest.php index a089059725..0e58bf49e3 100644 --- a/tests/src/Core/Lock/CacheLockDriverTest.php +++ b/tests/src/Core/Lock/ArrayCacheLockDriverTest.php @@ -6,7 +6,7 @@ namespace Friendica\Test\src\Core\Lock; use Friendica\Core\Cache\ArrayCache; use Friendica\Core\Lock\CacheLockDriver; -class CacheLockDriverTest extends LockTest +class ArrayCacheLockDriverTest extends LockTest { /** * @var \Friendica\Core\Cache\IMemoryCacheDriver @@ -24,4 +24,4 @@ class CacheLockDriverTest extends LockTest $this->cache->clear(); parent::tearDown(); } -} \ No newline at end of file +} diff --git a/tests/src/Core/Lock/DatabaseLockDriverTest.php b/tests/src/Core/Lock/DatabaseLockDriverTest.php index a80ff4c37c..f55ab0f9e2 100644 --- a/tests/src/Core/Lock/DatabaseLockDriverTest.php +++ b/tests/src/Core/Lock/DatabaseLockDriverTest.php @@ -11,49 +11,6 @@ use PHPUnit_Extensions_Database_DB_IDatabaseConnection; class DatabaseLockDriverTest extends LockTest { - use TestCaseTrait; - - /** - * Get database connection. - * - * This function is executed before each test in order to get a database connection that can be used by tests. - * If no prior connection is available, it tries to create one using the USER, PASS and DB environment variables. - * - * If it could not connect to the database, the test is skipped. - * - * @return PHPUnit_Extensions_Database_DB_IDatabaseConnection - * @see https://phpunit.de/manual/5.7/en/database.html - */ - protected function getConnection() - { - if (!dba::$connected) { - dba::connect('localhost', getenv('USER'), getenv('PASS'), getenv('DB')); - - if (dba::$connected) { - $app = get_app(); - // We need to do this in order to disable logging - $app->module = 'install'; - - // Create database structure - DBStructure::update(false, true, true); - } else { - $this->markTestSkipped('Could not connect to the database.'); - } - } - - return $this->createDefaultDBConnection(dba::get_db(), getenv('DB')); - } - - /** - * Get dataset to populate the database with. - * @return YamlDataSet - * @see https://phpunit.de/manual/5.7/en/database.html - */ - protected function getDataSet() - { - return new YamlDataSet(__DIR__ . '/../../../datasets/api.yml'); - } - protected function getInstance() { return new DatabaseLockDriver(); @@ -64,4 +21,4 @@ class DatabaseLockDriverTest extends LockTest dba::delete('locks', [ 'id > 0']); parent::tearDown(); } -} \ No newline at end of file +} diff --git a/tests/src/Core/Lock/LockTest.php b/tests/src/Core/Lock/LockTest.php index c8c0c32ae2..a120edeb0c 100644 --- a/tests/src/Core/Lock/LockTest.php +++ b/tests/src/Core/Lock/LockTest.php @@ -4,9 +4,10 @@ namespace Friendica\Test\src\Core\Lock; use Friendica\App; use Friendica\Core\Config; +use Friendica\Test\DatabaseTest; use PHPUnit\Framework\TestCase; -abstract class LockTest extends TestCase +abstract class LockTest extends DatabaseTest { /** * @var \Friendica\Core\Lock\ILockDriver @@ -58,6 +59,10 @@ abstract class LockTest extends TestCase $this->instance->acquireLock('bar', 1); $this->instance->acquireLock('nice', 1); + $this->assertTrue($this->instance->isLocked('foo')); + $this->assertTrue($this->instance->isLocked('bar')); + $this->assertTrue($this->instance->isLocked('nice')); + $this->instance->releaseAll(); $this->assertFalse($this->instance->isLocked('foo')); @@ -72,9 +77,13 @@ abstract class LockTest extends TestCase $this->instance->releaseLock('foo'); + $this->assertFalse($this->instance->isLocked('foo')); + $this->assertTrue($this->instance->isLocked('bar')); + $this->assertTrue($this->instance->isLocked('nice')); + $this->instance->releaseAll(); $this->assertFalse($this->instance->isLocked('bar')); - $this->assertFalse($this->instance->isLocked('#/$%ยง')); + $this->assertFalse($this->instance->isLocked('nice')); } -} \ No newline at end of file +} diff --git a/tests/src/Core/Lock/MemcachedCacheLockDriverTest.php b/tests/src/Core/Lock/MemcachedCacheLockDriverTest.php new file mode 100644 index 0000000000..678c54c77a --- /dev/null +++ b/tests/src/Core/Lock/MemcachedCacheLockDriverTest.php @@ -0,0 +1,40 @@ +cache = CacheDriverFactory::create('memcached'); + } catch (\Exception $exception) { + print "Redis - TestCase failed: " . $exception->getMessage(); + throw new \Exception(); + } + return new CacheLockDriver($this->cache); + } else { + $this->markTestSkipped('Redis driver isn\'t available'); + return null; + } + } + + public function tearDown() + { + if (class_exists('Redis')) { + $this->cache->clear(); + } + parent::tearDown(); + } +} diff --git a/tests/src/Core/Lock/RedisCacheLockDriverTest.php b/tests/src/Core/Lock/RedisCacheLockDriverTest.php new file mode 100644 index 0000000000..82d9b50def --- /dev/null +++ b/tests/src/Core/Lock/RedisCacheLockDriverTest.php @@ -0,0 +1,40 @@ +cache = CacheDriverFactory::create('redis'); + } catch (\Exception $exception) { + print "Redis - TestCase failed: " . $exception->getMessage(); + throw new \Exception(); + } + return new CacheLockDriver($this->cache); + } else { + $this->markTestSkipped('Redis driver isn\'t available'); + return null; + } + } + + public function tearDown() + { + if (class_exists('Redis')) { + $this->cache->clear(); + } + parent::tearDown(); + } +} diff --git a/tests/src/Core/Lock/SemaphoreLockDriverTest.php b/tests/src/Core/Lock/SemaphoreLockDriverTest.php index 56c96458f2..48c2aff4df 100644 --- a/tests/src/Core/Lock/SemaphoreLockDriverTest.php +++ b/tests/src/Core/Lock/SemaphoreLockDriverTest.php @@ -23,4 +23,4 @@ class SemaphoreLockDriverTest extends LockTest $this->semaphoreLockDriver->releaseAll(); parent::tearDown(); } -} \ No newline at end of file +}