Prevent multiple execution of aborted jobs #528
Conversation
|
@samdark Thanks for the feedback. I still need to investigate why the tests are failing. After that, I'll adapt the CHANGELOG. |
src/Queue.php
Outdated
| if ($job instanceof RetryableJobInterface && !$job->canRetry($attempt - 1, $error)) { | ||
| return true; | ||
| } else { | ||
| // Non RetryableJobs can have a maximum of one attempt |
There was a problem hiding this comment.
This assumption is incorrect. RetryableJobInterface means that job can decide whether it should be retried. In other case it is controlled by attempts property at queue level - job can still have multiple retries even if it doesn't implement RetryableJobInterface.
There was a problem hiding this comment.
@rob006 Thanks for pointing that out. I've adjusted the condition.
However, I still need to check the tests.
|
I was able to fix the tests. Unfortunately, PHP 5.4 and PHP 7 fail for other reasons. I don't have time to investigate this further at the moment. I'm not entirely happy with my fix/PR, as these failed jobs are closed without any error message. Unfortunately, no specific error is available either. At least these jobs are not executed multiple times. |
src/Queue.php
Outdated
| @@ -225,6 +225,16 @@ public function getWorkerPid() | |||
| protected function handleMessage($id, $message, $ttr, $attempt) | |||
| { | |||
| list($job, $error) = $this->unserializeMessage($message); | |||
|
|
|||
| // Handle aborted jobs without thrown error | |||
| if ($attempt > 1) { | |||
There was a problem hiding this comment.
I think that all this can be combined into one condition
There was a problem hiding this comment.
@s1lver Shall I summarize that? I'd be happy to. I personally prefer the breakdown for quick readability.
There was a problem hiding this comment.
What about this solution? Will return true if any of the conditions evaluate to true
return
($job instanceof RetryableJobInterface && !$job->canRetry($attempt - 1, $error))
|| (!($job instanceof RetryableJobInterface) && $attempt > $this->attempts)
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
Co-authored-by: Alexander Makarov <sam@rmcreative.ru>
|
Thank you! |
We have the situation that apparently in some environments, jobs canceled by
max_execution_timeare not correctly removed from the queue. As a result, they are executed again and again. Even if noRetryableJobInterfaceis implemented orcanRetry()always returnsfalse.Unfortunately, I can't reproduce it myself, but some users can in connection with CPanel environments. Perhaps the worker is there not running in CLI but in CGI mode.
In our project, we have now successfully solved the problem using the
ExecEvent. But maybe it makes sense to add this fix (even if I am not completely happy with it) to the upstream queue project.Related issue: humhub/calendar#547