From 679682c186ee99c98fe795721b5e620e03d96d6b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Tue, 10 Jan 2023 18:20:31 +0100 Subject: [PATCH 1/4] Use a Generator for job list to fix background-job:list command MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- core/Command/Background/ListCommand.php | 13 +++++++------ lib/private/BackgroundJob/JobList.php | 20 +++++++++++--------- lib/public/BackgroundJob/IJobList.php | 8 ++++---- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/core/Command/Background/ListCommand.php b/core/Command/Background/ListCommand.php index bdd45f3a25f..30440b226de 100644 --- a/core/Command/Background/ListCommand.php +++ b/core/Command/Background/ListCommand.php @@ -72,15 +72,16 @@ class ListCommand extends Base { return 0; } - protected function formatJobs(array $jobs): array { - return array_map( - fn ($job) => [ + protected function formatJobs(iterable $jobs): array { + $jobsInfo = []; + foreach ($jobs as $job) { + $jobsInfo[] = [ 'id' => $job->getId(), 'class' => get_class($job), 'last_run' => date(DATE_ATOM, $job->getLastRun()), 'argument' => json_encode($job->getArgument()), - ], - $jobs - ); + ]; + } + return $jobsInfo; } } diff --git a/lib/private/BackgroundJob/JobList.php b/lib/private/BackgroundJob/JobList.php index 20176e45125..3886ff13bc6 100644 --- a/lib/private/BackgroundJob/JobList.php +++ b/lib/private/BackgroundJob/JobList.php @@ -160,19 +160,19 @@ class JobList implements IJobList { /** * get all jobs in the list * - * @return IJob[] + * @return iterable * @deprecated 9.0.0 - This method is dangerous since it can cause load and * memory problems when creating too many instances. Use getJobs instead. */ - public function getAll(): array { + public function getAll(): iterable { return $this->getJobs(null, null, 0); } /** * @param IJob|class-string|null $job - * @return IJob[] + * @return iterable Avoid to store these objects as they may share a Singleton instance. You should instead use these IJobs instances while looping on the iterable. */ - public function getJobs($job, ?int $limit, int $offset): array { + public function getJobs($job, ?int $limit, int $offset): iterable { $query = $this->connection->getQueryBuilder(); $query->select('*') ->from('jobs') @@ -190,20 +190,18 @@ class JobList implements IJobList { $result = $query->executeQuery(); - $jobs = []; while ($row = $result->fetch()) { $job = $this->buildJob($row); if ($job) { - $jobs[] = $job; + yield $job; } } $result->closeCursor(); - - return $jobs; } /** - * get the next job in the list + * Get the next job in the list + * @return ?IJob the next job to run. Beware that this object may be a singleton and may be modified by the next call to buildJob. */ public function getNext(bool $onlyTimeSensitive = false): ?IJob { $query = $this->connection->getQueryBuilder(); @@ -261,6 +259,9 @@ class JobList implements IJobList { } } + /** + * @return ?IJob The job matching the id. Beware that this object may be a singleton and may be modified by the next call to buildJob. + */ public function getById(int $id): ?IJob { $row = $this->getDetailsById($id); @@ -291,6 +292,7 @@ class JobList implements IJobList { * get the job object from a row in the db * * @param array{class:class-string, id:mixed, last_run:mixed, argument:string} $row + * @return ?IJob the next job to run. Beware that this object may be a singleton and may be modified by the next call to buildJob. */ private function buildJob(array $row): ?IJob { try { diff --git a/lib/public/BackgroundJob/IJobList.php b/lib/public/BackgroundJob/IJobList.php index 8f6449b070b..bd43f0771f6 100644 --- a/lib/public/BackgroundJob/IJobList.php +++ b/lib/public/BackgroundJob/IJobList.php @@ -78,21 +78,21 @@ interface IJobList { /** * get all jobs in the list * - * @return IJob[] + * @return iterable * @since 7.0.0 * @deprecated 9.0.0 - This method is dangerous since it can cause load and * memory problems when creating too many instances. Use getJobs instead. */ - public function getAll(): array; + public function getAll(): iterable; /** * Get jobs matching the search * * @param IJob|class-string|null $job - * @return IJob[] + * @return iterable * @since 25.0.0 */ - public function getJobs($job, ?int $limit, int $offset): array; + public function getJobs($job, ?int $limit, int $offset): iterable; /** * get the next job in the list From 75fca38e665681f3ef4e87b41a1910e47ba6c90e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 12 Jan 2023 11:40:18 +0100 Subject: [PATCH 2/4] Fix BackgroundJob list tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- tests/lib/BackgroundJob/JobListTest.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/lib/BackgroundJob/JobListTest.php b/tests/lib/BackgroundJob/JobListTest.php index 736d670ed20..62f04509068 100644 --- a/tests/lib/BackgroundJob/JobListTest.php +++ b/tests/lib/BackgroundJob/JobListTest.php @@ -54,7 +54,12 @@ class JobListTest extends TestCase { } protected function getAllSorted() { - $jobs = $this->instance->getAll(); + $iterator = $this->instance->getJobs(null, null, 0); + $jobs = []; + + foreach ($iterator as $job) { + $jobs[] = clone $job; + } usort($jobs, function (IJob $job1, IJob $job2) { return $job1->getId() - $job2->getId(); From e74f4646622a6f667228d52be1dde091bb0b2757 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 12 Jan 2023 17:06:23 +0100 Subject: [PATCH 3/4] Remove deprecated method getAll instead of breaking API on deprecated method MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- lib/private/BackgroundJob/JobList.php | 11 ----------- lib/public/BackgroundJob/IJobList.php | 10 ---------- 2 files changed, 21 deletions(-) diff --git a/lib/private/BackgroundJob/JobList.php b/lib/private/BackgroundJob/JobList.php index 3886ff13bc6..494735ad873 100644 --- a/lib/private/BackgroundJob/JobList.php +++ b/lib/private/BackgroundJob/JobList.php @@ -157,17 +157,6 @@ class JobList implements IJobList { return (bool) $row; } - /** - * get all jobs in the list - * - * @return iterable - * @deprecated 9.0.0 - This method is dangerous since it can cause load and - * memory problems when creating too many instances. Use getJobs instead. - */ - public function getAll(): iterable { - return $this->getJobs(null, null, 0); - } - /** * @param IJob|class-string|null $job * @return iterable Avoid to store these objects as they may share a Singleton instance. You should instead use these IJobs instances while looping on the iterable. diff --git a/lib/public/BackgroundJob/IJobList.php b/lib/public/BackgroundJob/IJobList.php index bd43f0771f6..0b32607feb6 100644 --- a/lib/public/BackgroundJob/IJobList.php +++ b/lib/public/BackgroundJob/IJobList.php @@ -75,16 +75,6 @@ interface IJobList { */ public function has($job, $argument): bool; - /** - * get all jobs in the list - * - * @return iterable - * @since 7.0.0 - * @deprecated 9.0.0 - This method is dangerous since it can cause load and - * memory problems when creating too many instances. Use getJobs instead. - */ - public function getAll(): iterable; - /** * Get jobs matching the search * From d74044f6341e93861d820f21b6d37a2f98e1ef60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?C=C3=B4me=20Chilliet?= Date: Thu, 12 Jan 2023 17:18:59 +0100 Subject: [PATCH 4/4] Fix API breakage by using a new method instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Côme Chilliet --- core/Command/Background/ListCommand.php | 2 +- lib/private/BackgroundJob/JobList.php | 11 ++++++++++- lib/public/BackgroundJob/IJobList.php | 14 ++++++++++++-- tests/lib/BackgroundJob/DummyJobList.php | 2 +- tests/lib/BackgroundJob/JobListTest.php | 2 +- 5 files changed, 25 insertions(+), 6 deletions(-) diff --git a/core/Command/Background/ListCommand.php b/core/Command/Background/ListCommand.php index 30440b226de..4116bfa0ff1 100644 --- a/core/Command/Background/ListCommand.php +++ b/core/Command/Background/ListCommand.php @@ -67,7 +67,7 @@ class ListCommand extends Base { } protected function execute(InputInterface $input, OutputInterface $output): int { - $jobs = $this->jobList->getJobs($input->getOption('class'), (int)$input->getOption('limit'), (int)$input->getOption('offset')); + $jobs = $this->jobList->getJobsIterator($input->getOption('class'), (int)$input->getOption('limit'), (int)$input->getOption('offset')); $this->writeTableInOutputFormat($input, $output, $this->formatJobs($jobs)); return 0; } diff --git a/lib/private/BackgroundJob/JobList.php b/lib/private/BackgroundJob/JobList.php index 494735ad873..67b736b8dd9 100644 --- a/lib/private/BackgroundJob/JobList.php +++ b/lib/private/BackgroundJob/JobList.php @@ -157,11 +157,20 @@ class JobList implements IJobList { return (bool) $row; } + public function getJobs($job, ?int $limit, int $offset): array { + $iterable = $this->getJobsIterator($job, $limit, $offset); + if (is_array($iterable)) { + return $iterable; + } else { + return iterator_to_array($iterable); + } + } + /** * @param IJob|class-string|null $job * @return iterable Avoid to store these objects as they may share a Singleton instance. You should instead use these IJobs instances while looping on the iterable. */ - public function getJobs($job, ?int $limit, int $offset): iterable { + public function getJobsIterator($job, ?int $limit, int $offset): iterable { $query = $this->connection->getQueryBuilder(); $query->select('*') ->from('jobs') diff --git a/lib/public/BackgroundJob/IJobList.php b/lib/public/BackgroundJob/IJobList.php index 0b32607feb6..e8d0380e604 100644 --- a/lib/public/BackgroundJob/IJobList.php +++ b/lib/public/BackgroundJob/IJobList.php @@ -79,10 +79,20 @@ interface IJobList { * Get jobs matching the search * * @param IJob|class-string|null $job - * @return iterable + * @return array * @since 25.0.0 + * @deprecated 26.0.0 Use getJobsIterator instead to avoid duplicated job objects */ - public function getJobs($job, ?int $limit, int $offset): iterable; + public function getJobs($job, ?int $limit, int $offset): array; + + /** + * Get jobs matching the search + * + * @param IJob|class-string|null $job + * @return iterable + * @since 26.0.0 + */ + public function getJobsIterator($job, ?int $limit, int $offset): iterable; /** * get the next job in the list diff --git a/tests/lib/BackgroundJob/DummyJobList.php b/tests/lib/BackgroundJob/DummyJobList.php index 4d14ed9e7db..be48259789a 100644 --- a/tests/lib/BackgroundJob/DummyJobList.php +++ b/tests/lib/BackgroundJob/DummyJobList.php @@ -72,7 +72,7 @@ class DummyJobList extends \OC\BackgroundJob\JobList { return $this->jobs; } - public function getJobs($job, ?int $limit, int $offset): array { + public function getJobsIterator($job, ?int $limit, int $offset): array { if ($job instanceof IJob) { $jobClass = get_class($job); } else { diff --git a/tests/lib/BackgroundJob/JobListTest.php b/tests/lib/BackgroundJob/JobListTest.php index 62f04509068..ea02e1cd8b9 100644 --- a/tests/lib/BackgroundJob/JobListTest.php +++ b/tests/lib/BackgroundJob/JobListTest.php @@ -54,7 +54,7 @@ class JobListTest extends TestCase { } protected function getAllSorted() { - $iterator = $this->instance->getJobs(null, null, 0); + $iterator = $this->instance->getJobsIterator(null, null, 0); $jobs = []; foreach ($iterator as $job) {