Merge pull request #36073 from nextcloud/fix/fix-background-job-list

Use a Generator for job list to fix background-job:list command

core/Command/Background/ListCommand.php

@@ -67,20 +67,21 @@ 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;
 	}
 
-	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;
 	}
 }

lib/private/BackgroundJob/JobList.php

@@ -157,22 +157,20 @@ class JobList implements IJobList {
 		return (bool) $row;
 	}
 
-	/**
-	 * get all jobs in the list
-	 *
-	 * @return IJob[]
-	 * @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 {
-		return $this->getJobs(null, null, 0);
+	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<IJob>|null $job
-	 * @return IJob[]
+	 * @return iterable<IJob> 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 getJobsIterator($job, ?int $limit, int $offset): iterable {
 		$query = $this->connection->getQueryBuilder();
 		$query->select('*')
 			->from('jobs')
@@ -190,20 +188,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 +257,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 +290,7 @@ class JobList implements IJobList {
 	 * get the job object from a row in the db
 	 *
 	 * @param array{class:class-string<IJob>, 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 {

lib/public/BackgroundJob/IJobList.php

@@ -76,23 +76,23 @@ interface IJobList {
 	public function has($job, $argument): bool;
 
 	/**
-	 * get all jobs in the list
+	 * Get jobs matching the search
 	 *
-	 * @return IJob[]
-	 * @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.
+	 * @param IJob|class-string<IJob>|null $job
+	 * @return array<IJob>
+	 * @since 25.0.0
+	 * @deprecated 26.0.0 Use getJobsIterator instead to avoid duplicated job objects
 	 */
-	public function getAll(): array;
+	public function getJobs($job, ?int $limit, int $offset): array;
 
 	/**
 	 * Get jobs matching the search
 	 *
 	 * @param IJob|class-string<IJob>|null $job
-	 * @return IJob[]
-	 * @since 25.0.0
+	 * @return iterable<IJob>
+	 * @since 26.0.0
 	 */
-	public function getJobs($job, ?int $limit, int $offset): array;
+	public function getJobsIterator($job, ?int $limit, int $offset): iterable;
 
 	/**
 	 * get the next job in the list

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 {

tests/lib/BackgroundJob/JobListTest.php

@@ -54,7 +54,12 @@ class JobListTest extends TestCase {
 	}
 
 	protected function getAllSorted() {
-		$jobs = $this->instance->getAll();
+		$iterator = $this->instance->getJobsIterator(null, null, 0);
+		$jobs = [];
+
+		foreach ($iterator as $job) {
+			$jobs[] = clone $job;
+		}
 
 		usort($jobs, function (IJob $job1, IJob $job2) {
 			return $job1->getId() - $job2->getId();