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):
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.