From f148dcabc45c2c793f28518aa2deec6d51826546 Mon Sep 17 00:00:00 2001 From: Philipp Holzer Date: Wed, 5 Sep 2018 22:16:23 +0200 Subject: [PATCH 01/20] 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 02/20] 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'), + ]); } /** From 807ad145218ab6d4c57b6d200f4c89f63620502f Mon Sep 17 00:00:00 2001 From: Jonny Tischbein Date: Wed, 19 Sep 2018 23:55:29 +0200 Subject: [PATCH 03/20] calculate form return_url for post and message instead of using SESSION var --- mod/editpost.php | 8 +++++++- mod/message.php | 2 +- src/Object/Post.php | 2 +- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/mod/editpost.php b/mod/editpost.php index 258585ef1c..61dfcf911d 100644 --- a/mod/editpost.php +++ b/mod/editpost.php @@ -21,12 +21,18 @@ function editpost_content(App $a) } $post_id = (($a->argc > 1) ? intval($a->argv[1]) : 0); + $return_url = (($a->argc > 1) ? base64_decode($a->argv[2]) : ''); if (!$post_id) { notice(L10n::t('Item not found') . EOL); return; } + // Fallback to SESSION return_path + if (empty($return_url)) { + $return_url = $_SESSION['return_path']; + } + $fields = ['allow_cid', 'allow_gid', 'deny_cid', 'deny_gid', 'type', 'body', 'title', 'file', 'wall', 'post-type']; @@ -95,7 +101,7 @@ function editpost_content(App $a) $o .= replace_macros($tpl, [ '$is_edit' => true, - '$return_path' => $_SESSION['return_url'], + '$return_path' => $return_url, '$action' => 'item', '$share' => L10n::t('Save'), '$upload' => L10n::t('Upload photo'), diff --git a/mod/message.php b/mod/message.php index 8c9aa657df..9ac0ddbc0b 100644 --- a/mod/message.php +++ b/mod/message.php @@ -92,7 +92,7 @@ function message_post(App $a) $a->argc = 2; $a->argv[1] = 'new'; } else { - goaway($_SESSION['return_url']); + goaway(System::baseUrl() . '/' . $a->cmd . '/' . $ret); } } diff --git a/src/Object/Post.php b/src/Object/Post.php index 038ca270d7..c52078e521 100644 --- a/src/Object/Post.php +++ b/src/Object/Post.php @@ -157,7 +157,7 @@ class Post extends BaseObject if ($item["event-id"] != 0) { $edpost = ["events/event/" . $item['event-id'], L10n::t("Edit")]; } else { - $edpost = ["editpost/" . $item['id'], L10n::t("Edit")]; + $edpost = ["editpost/" . $item['id'] . "/" . base64_encode($a->cmd), L10n::t("Edit")]; } $dropping = in_array($item['uid'], [0, local_user()]); } else { From 58f26d195f5b89c7276395a145b58e10917d085e Mon Sep 17 00:00:00 2001 From: Jonny Tischbein Date: Thu, 20 Sep 2018 14:41:52 +0200 Subject: [PATCH 04/20] adjust argc check in mod/editpost + relative path in mod/message --- mod/editpost.php | 2 +- mod/message.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/mod/editpost.php b/mod/editpost.php index 61dfcf911d..d4f5567eeb 100644 --- a/mod/editpost.php +++ b/mod/editpost.php @@ -21,7 +21,7 @@ function editpost_content(App $a) } $post_id = (($a->argc > 1) ? intval($a->argv[1]) : 0); - $return_url = (($a->argc > 1) ? base64_decode($a->argv[2]) : ''); + $return_url = (($a->argc > 2) ? base64_decode($a->argv[2]) : ''); if (!$post_id) { notice(L10n::t('Item not found') . EOL); diff --git a/mod/message.php b/mod/message.php index 9ac0ddbc0b..7377ba5e48 100644 --- a/mod/message.php +++ b/mod/message.php @@ -92,7 +92,7 @@ function message_post(App $a) $a->argc = 2; $a->argv[1] = 'new'; } else { - goaway(System::baseUrl() . '/' . $a->cmd . '/' . $ret); + goaway($a->cmd . '/' . $ret); } } From 0f63a6276127f9e76bc11e70c03604ed33b6b9b0 Mon Sep 17 00:00:00 2001 From: Jonny Tischbein Date: Thu, 20 Sep 2018 15:55:24 +0200 Subject: [PATCH 05/20] fix return fallback to SESSION Variable --- mod/editpost.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mod/editpost.php b/mod/editpost.php index d4f5567eeb..9f4aee31d6 100644 --- a/mod/editpost.php +++ b/mod/editpost.php @@ -30,7 +30,7 @@ function editpost_content(App $a) // Fallback to SESSION return_path if (empty($return_url)) { - $return_url = $_SESSION['return_path']; + $return_url = $_SESSION['return_url']; } $fields = ['allow_cid', 'allow_gid', 'deny_cid', 'deny_gid', From b1720c07ec963fa5fa3800911068ad680404f792 Mon Sep 17 00:00:00 2001 From: Jonny Tischbein Date: Thu, 20 Sep 2018 16:20:33 +0200 Subject: [PATCH 06/20] Add Share button for preview new post --- view/theme/frio/templates/jot.tpl | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/view/theme/frio/templates/jot.tpl b/view/theme/frio/templates/jot.tpl index 423be5a5d9..c4ccdfd864 100644 --- a/view/theme/frio/templates/jot.tpl +++ b/view/theme/frio/templates/jot.tpl @@ -117,7 +117,12 @@ {{$acl}} - + + From 4c56724715d1e87517cb8ae2fd79accc18f19394 Mon Sep 17 00:00:00 2001 From: Jonny Tischbein Date: Thu, 20 Sep 2018 16:50:58 +0200 Subject: [PATCH 07/20] make share button visible in preview --- view/theme/frio/templates/jot.tpl | 3 +++ 1 file changed, 3 insertions(+) diff --git a/view/theme/frio/templates/jot.tpl b/view/theme/frio/templates/jot.tpl index c4ccdfd864..7a4854ce44 100644 --- a/view/theme/frio/templates/jot.tpl +++ b/view/theme/frio/templates/jot.tpl @@ -117,6 +117,9 @@ {{$acl}} + + +