From cdb61be06fd8571d1cbd18c875b8f69ea01295a0 Mon Sep 17 00:00:00 2001 From: Philipp Date: Tue, 15 Sep 2020 18:16:44 +0200 Subject: [PATCH 1/3] Rewrite Process Model/Core --- bin/worker.php | 3 ++- src/Core/Process.php | 45 +++++++++++++++++++++++++++++++++---- src/Core/Worker.php | 30 +++---------------------- src/DI.php | 7 ++++++ src/Model/Process.php | 52 +++++++++++++++++++++---------------------- src/Module/Worker.php | 5 +++-- 6 files changed, 82 insertions(+), 60 deletions(-) diff --git a/bin/worker.php b/bin/worker.php index 1b70a2095..46eff1071 100755 --- a/bin/worker.php +++ b/bin/worker.php @@ -23,6 +23,7 @@ use Dice\Dice; use Friendica\App; +use Friendica\Core\Process; use Friendica\Core\Update; use Friendica\Core\Worker; use Friendica\DI; @@ -76,4 +77,4 @@ Worker::processQueue($run_cron); Worker::unclaimProcess(); -Worker::endProcess(); +DI::process()->end(); diff --git a/src/Core/Process.php b/src/Core/Process.php index fd6b17fe1..08284b211 100644 --- a/src/Core/Process.php +++ b/src/Core/Process.php @@ -23,6 +23,7 @@ namespace Friendica\Core; use Friendica\App; use Friendica\Core\Config\IConfig; +use Friendica\Model; use Psr\Log\LoggerInterface; /** @@ -56,12 +57,48 @@ class Process */ private $basePath; - public function __construct(LoggerInterface $logger, App\Mode $mode, IConfig $config, string $basepath) + /** @var Model\Process */ + private $processModel; + + /** + * The Process ID of this process + * + * @var int + */ + private $pid; + + public function __construct(LoggerInterface $logger, App\Mode $mode, IConfig $config, Model\Process $processModel, string $basepath) { - $this->logger = $logger; - $this->mode = $mode; - $this->config = $config; + $this->logger = $logger; + $this->mode = $mode; + $this->config = $config; $this->basePath = $basepath; + $this->processModel = $processModel; + $this->pid = getmypid(); + } + + /** + * Log active processes into the "process" table + */ + public function start() + { + $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 1); + + $command = basename($trace[0]['file']); + + $this->processModel->deleteInactive(); + $this->processModel->insert($command, $this->pid); + } + + /** + * Remove the active process from the "process" table + * + * @return bool + * @throws \Exception + */ + public function end() + { + return $this->processModel->deleteByPid($this->pid); } /** diff --git a/src/Core/Worker.php b/src/Core/Worker.php index 7758e06a9..80ec16982 100644 --- a/src/Core/Worker.php +++ b/src/Core/Worker.php @@ -22,6 +22,7 @@ namespace Friendica\Core; use Friendica\Core; +use Friendica\Core\Process as ProcessAlias; use Friendica\Database\DBA; use Friendica\DI; use Friendica\Model\Process; @@ -72,7 +73,7 @@ class Worker } // We now start the process. This is done after the load check since this could increase the load. - self::startProcess(); + DI::process()->start(); // Kill stale processes every 5 minutes $last_cleanup = DI::config()->get('system', 'worker_last_cleaned', 0); @@ -1092,7 +1093,7 @@ class Worker if (self::tooMuchWorkers()) { // Cleaning dead processes self::killStaleWorkers(); - Process::deleteInactive(); + DI::modelProcess()->deleteInactive(); return; } @@ -1360,31 +1361,6 @@ class Worker return true; } - /** - * Log active processes into the "process" table - */ - public static function startProcess() - { - $trace = debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 1); - - $command = basename($trace[0]['file']); - - Process::deleteInactive(); - - Process::insert($command); - } - - /** - * Remove the active process from the "process" table - * - * @return bool - * @throws \Exception - */ - public static function endProcess() - { - return Process::deleteByPid(); - } - /** * Set the flag if some job is waiting * diff --git a/src/DI.php b/src/DI.php index cb6166692..4db53f271 100644 --- a/src/DI.php +++ b/src/DI.php @@ -314,6 +314,13 @@ abstract class DI // // "Model" namespace instances // + /** + * @return Model\Process + */ + public static function modelProcess() + { + return self::$dice->create(Model\Process::class); + } /** * @return Model\User\Cookie diff --git a/src/Model/Process.php b/src/Model/Process.php index 18b5f785a..cc8a18429 100644 --- a/src/Model/Process.php +++ b/src/Model/Process.php @@ -21,7 +21,7 @@ namespace Friendica\Model; -use Friendica\Database\DBA; +use Friendica\Database\Database; use Friendica\Util\DateTimeFormat; /** @@ -29,29 +29,33 @@ use Friendica\Util\DateTimeFormat; */ class Process { + /** @var Database */ + private $dba; + + public function __construct(Database $dba) + { + $this->dba = $dba; + } + /** * Insert a new process row. If the pid parameter is omitted, we use the current pid * * @param string $command - * @param string $pid + * @param int $pid The process id to insert * @return bool * @throws \Exception */ - public static function insert($command, $pid = null) + public function insert(string $command, int $pid) { $return = true; - if (is_null($pid)) { - $pid = getmypid(); + $this->dba->transaction(); + + if (!$this->dba->exists('process', ['pid' => $pid])) { + $return = $this->dba->insert('process', ['pid' => $pid, 'command' => $command, 'created' => DateTimeFormat::utcNow()]); } - DBA::transaction(); - - if (!DBA::exists('process', ['pid' => $pid])) { - $return = DBA::insert('process', ['pid' => $pid, 'command' => $command, 'created' => DateTimeFormat::utcNow()]); - } - - DBA::commit(); + $this->dba->commit(); return $return; } @@ -59,33 +63,29 @@ class Process /** * Remove a process row by pid. If the pid parameter is omitted, we use the current pid * - * @param string $pid + * @param int $pid The pid to delete * @return bool * @throws \Exception */ - public static function deleteByPid($pid = null) + public function deleteByPid(int $pid) { - if ($pid === null) { - $pid = getmypid(); - } - - return DBA::delete('process', ['pid' => $pid]); + return $this->dba->delete('process', ['pid' => $pid]); } /** * Clean the process table of inactive physical processes */ - public static function deleteInactive() + public function deleteInactive() { - DBA::transaction(); + $this->dba->transaction(); - $processes = DBA::select('process', ['pid']); - while($process = DBA::fetch($processes)) { + $processes = $this->dba->select('process', ['pid']); + while($process = $this->dba->fetch($processes)) { if (!posix_kill($process['pid'], 0)) { - self::deleteByPid($process['pid']); + $this->deleteByPid($process['pid']); } } - DBA::close($processes); - DBA::commit(); + $this->dba->close($processes); + $this->dba->commit(); } } diff --git a/src/Module/Worker.php b/src/Module/Worker.php index bd06c4029..d73ed1d8e 100644 --- a/src/Module/Worker.php +++ b/src/Module/Worker.php @@ -22,6 +22,7 @@ namespace Friendica\Module; use Friendica\BaseModule; +use Friendica\Core\Process; use Friendica\Core\System; use Friendica\Core\Worker as WorkerCore; use Friendica\Database\DBA; @@ -57,7 +58,7 @@ class Worker extends BaseModule return; } - WorkerCore::startProcess(); + DI::process()->start(); DI::logger()->notice('Front end worker started.', ['pid' => getmypid()]); @@ -79,7 +80,7 @@ class Worker extends BaseModule WorkerCore::unclaimProcess(); - WorkerCore::endProcess(); + DI::process()->end(); System::httpExit(200, 'Frontend worker stopped.'); } From 7413b362f5850acd16807b4b49d721a5c840afcb Mon Sep 17 00:00:00 2001 From: Philipp Date: Thu, 17 Sep 2020 22:15:33 +0200 Subject: [PATCH 2/3] Add basic process tests --- tests/src/Model/ProcessTest.php | 81 +++++++++++++++++++++++++++++ tests/src/Model/User/CookieTest.php | 6 +-- 2 files changed, 84 insertions(+), 3 deletions(-) create mode 100644 tests/src/Model/ProcessTest.php diff --git a/tests/src/Model/ProcessTest.php b/tests/src/Model/ProcessTest.php new file mode 100644 index 000000000..389e2ac70 --- /dev/null +++ b/tests/src/Model/ProcessTest.php @@ -0,0 +1,81 @@ +setUpVfsDir(); + + $this->logger = new NullLogger(); + + $profiler = \Mockery::mock(Profiler::class); + $profiler->shouldReceive('saveTimestamp')->withAnyArgs()->andReturn(true); + + // load real config to avoid mocking every config-entry which is related to the Database class + $configFactory = new ConfigFactory(); + $loader = new ConfigFileLoader($this->root->url()); + $configCache = $configFactory->createCache($loader); + + $this->dba = new StaticDatabase($configCache, $profiler, $this->logger); + } + + public function testInsertDelete() + { + $process = new Process($this->dba); + + $this->assertEquals(0, $this->dba->count('process')); + $process->insert('test', 1); + $process->insert('test2', 2); + $process->insert('test3', 3); + + $this->assertEquals(3, $this->dba->count('process')); + + $this->assertEquals([ + ['command' => 'test'] + ], $this->dba->selectToArray('process', ['command'], ['pid' => 1])); + + $process->deleteByPid(1); + + $this->assertEmpty($this->dba->selectToArray('process', ['command'], ['pid' => 1])); + + $this->assertEquals(2, $this->dba->count('process')); + } + + public function testDoubleInsert() + { + $process = new Process($this->dba); + + $process->insert('test', 1); + + // double insert doesn't work + $process->insert('test23', 1); + + $this->assertEquals([['command' => 'test']], $this->dba->selectToArray('process', ['command'], ['pid' => 1])); + } + + public function testWrongDelete() + { + $process = new Process($this->dba); + + // Just ignore wrong deletes, no execution is thrown + $process->deleteByPid(-1); + } +} diff --git a/tests/src/Model/User/CookieTest.php b/tests/src/Model/User/CookieTest.php index cac278076..40c99f3ea 100644 --- a/tests/src/Model/User/CookieTest.php +++ b/tests/src/Model/User/CookieTest.php @@ -19,16 +19,16 @@ * */ -namespace Friendica\Testsrc\Model\User; +namespace Friendica\Test\src\Model\User; use Friendica\App\BaseURL; use Friendica\Core\Config\IConfig; use Friendica\Model\User\Cookie; -use Friendica\Test\DatabaseTest; +use Friendica\Test\MockedTest; use Friendica\Test\Util\StaticCookie; use Mockery\MockInterface; -class CookieTest extends DatabaseTest +class CookieTest extends MockedTest { /** @var MockInterface|IConfig */ private $config; From c9e510d4fdb74e6efc64b2c66e3975fdec4ed11f Mon Sep 17 00:00:00 2001 From: Philipp Date: Sat, 19 Sep 2020 20:28:01 +0200 Subject: [PATCH 3/3] Use getmypid() as dependency (improve testing) --- src/Core/Process.php | 4 ++-- src/Core/Worker.php | 2 +- static/dependencies.config.php | 1 + 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Core/Process.php b/src/Core/Process.php index 08284b211..f8958f102 100644 --- a/src/Core/Process.php +++ b/src/Core/Process.php @@ -67,14 +67,14 @@ class Process */ private $pid; - public function __construct(LoggerInterface $logger, App\Mode $mode, IConfig $config, Model\Process $processModel, string $basepath) + public function __construct(LoggerInterface $logger, App\Mode $mode, IConfig $config, Model\Process $processModel, string $basepath, int $pid) { $this->logger = $logger; $this->mode = $mode; $this->config = $config; $this->basePath = $basepath; $this->processModel = $processModel; - $this->pid = getmypid(); + $this->pid = $pid; } /** diff --git a/src/Core/Worker.php b/src/Core/Worker.php index 80ec16982..c586310b7 100644 --- a/src/Core/Worker.php +++ b/src/Core/Worker.php @@ -1172,7 +1172,7 @@ class Worker $args = ['no_cron' => !$do_cron]; $a = DI::app(); - $process = new Core\Process(DI::logger(), DI::mode(), DI::config(), $a->getBasePath()); + $process = new Core\Process(DI::logger(), DI::mode(), DI::config(), DI::modelProcess(), $a->getBasePath(), getmypid()); $process->run($command, $args); // after spawning we have to remove the flag. diff --git a/static/dependencies.config.php b/static/dependencies.config.php index 3df54b79e..b1a54786a 100644 --- a/static/dependencies.config.php +++ b/static/dependencies.config.php @@ -187,6 +187,7 @@ return [ Process::class => [ 'constructParams' => [ [Dice::INSTANCE => '$basepath'], + getmypid(), ], ], App\Router::class => [