dereuromark/cakephp-queue

Make consistent usage of `defaultworkertimeout` throughout the code

Closed this issue · 0 comments

It occured to me that the usage of the defaultworkertimeout is not consistent. IMHO, it should always be > 0, as it otherwise produces strange results in cleaning the process table:

In this case. a subSeconds(0) would basically remove all processes from the table
QueueProcessesTable.php#200

	public function cleanEndedProcesses(): int {
		$timeout = Config::defaultworkertimeout();
		$thresholdTime = (new DateTime())->subSeconds($timeout);

		return $this->deleteAll(['modified <' => $thresholdTime]);
	}

It seems also to be better to use the findActive() and use a NOT IN() clause to keep code dry (like a findNotActive)
QueueProcessesTable.php#134

	/**
	 * @return \Cake\ORM\Query\SelectQuery
	 */
	public function findActive(): SelectQuery {
		$timeout = Config::defaultworkertimeout();
		$thresholdTime = (new DateTime())->subSeconds($timeout);

		return $this->find()->where(['modified > ' => $thresholdTime]);
	}

But over here it seems it can be zero (or false):

WorkerCommand.php#189

protected function clean(ConsoleIo $io): int {
		$timeout = Config::defaultworkertimeout();
		if (!$timeout) {
			$io->abort('You disabled `defaultworkertimeout` in config. Aborting.');
		}
		$thresholdTime = (new DateTime())->subSeconds($timeout);

To me it make sense that a constrain is set, that the defaultworkertimeout is very high and should not be zero. Preferably check in the Config file and throw a warning message if 0 is used.

Then there is another usage of the Config::defaultworkertimeout() which I think is a bug, but not sure how it is ment to be:
WorkerCommand.php#189

if ($costConstraints || $uniqueConstraints) {
			$constraintJobs = array_keys($costConstraints + $uniqueConstraints);
			$runningJobs = $this->find('queued')
				->contain(['WorkerProcesses'])
				->where(['QueuedJobs.job_task IN' => $constraintJobs, 'QueuedJobs.workerkey IS NOT' => null, 'QueuedJobs.workerkey !=' => $this->_key, 'WorkerProcesses.modified >' => Config::defaultworkertimeout()])
				->all()
				->toArray();
		}

In this case, the WorkerProcesses.modified >' => Config::defaultworkertimeout() is almost always true, as seconds since 1970 leave us with all workerprocesses after 1970..
But, it this used as constrain? Then I propose we convert to a DateTime in the findActive query

I will post a PR with proposed fixes in minute or so.