RedisQueue::pop() has 2 issues

[halaei]

I am concerned about 2 issues in this code from RedisQueue.php:

public function pop($queue = null)
{
    $original = $queue ?: $this->default;
    $this->migrateAllExpiredJobs($queue = $this->getQueue($queue));
    $job = $this->redis->lpop($queue);
    if ( ! is_null($job))
    {
        $this->redis->zadd($queue.':reserved', $this->getTime() + 60, $job);
        return new RedisJob($this->container, $this, $job, $original);
    }
}

1. Assume you pop a job and then something crashed. Then the job cab be lost for ever before getting reserved. I guess this can be handled with Redis scripts or transactions (but not sure).

2. assume you run this command:

   php artisan queue:listen --timeout=120

This means you expect your job takes more than 1 min. Now if you have multiple listeners to the same queue (I don't see anybody told me I can't do that), then the job may get re-pushed by the execution of following line by another listener:

$this->migrateExpiredJobs($queue.':reserved', $queue);

I think it will be nice to replace that hard-coded 60 with something like $this->max_run_time + some_epsilon.

[sitzmar]

I can confirm scenario 2 does occur. When popping a job off the queue the timeout needs to be adjusted to allow jobs to run longer than the hard coded limit of 60 seconds.

[smazurov]

There is a proposed fix to this here: #6146

[miladr]

Taylor fixed this issue. Its nice to have an accurate developer in our team.

[halaei]

Issue 1 is not fixed yet.