Surface Redis exceptions instead of silently returning false

[chrisboulton]

This is a first pass at changes to be loud whenever an error talking to Redis occurs, which should hopefully address some of the concerns in #228.

There's no way to differentiate between a connection related error and a general error returned from Redis (key is not of that data type, etc), so here we just throw a generic Resque_RedisException for anything we see.

Still need to do a full pass over all the Redis usage to see what behavioral changes we can expect as a result of this.

[danhunsaker]

👍

[peterjmit]

👍 we have had a similar fix running in the app I mentioned in #228 for the last 5/6 days. No problems having implemented it yet (but also no reports of the exception being thrown, so ¯_(ツ)_/¯).

[peterjmit]

Logging (and other) libraries can/will provide more context/information if the exception is passed as the third argument to \Exception where Exception::getPrevious can be used.

For example

throw new Resque_RedisException('Some message', 0, $e)

In this case $e will be cast to a string and we will lose some of that additional reporting.

[chrisboulton]

Ah thanks - I totally forgot that was a thing. Have updated the PR.

[chrisboulton]

Great - I still need to spend a few moments checking the Redis usage elsewhere, so I won't merge for now.

[scaleupcto]

This really needs merging. We're loosing jobs in production if redis is out of memory, for example. Is there anything we can do to help this get merged?

[raajeshnew]

Has this been merged yet. We are loosing jobs too

[rajibahmed]

Hi raajeshnew,

If its really urgent, then you can fork the repo an merge it there. Update your composer file to php-resque to use your version. I think it will take awhile to Chris to any update on this library :) and its fairly stable lib.
Hope I have been helpful to last to commenter.

//Best

[scaleupcto]

So you would consider loosing jobs as non-urgent and/or not something a stable library might care about urgently? That's kind of bogus.

[scaleupcto]

Forking off the branch with the fix in here misses out all the other fixes that have gone into master since that branch was created, so I think that's a bad idea.

Instead, if anyone wants to see how we got around this without forking, then take a look at:

https://github.com/talis/tripod-php/blob/master/src/mongo/base/JobBase.class.php#L91

We use querying the status to see if the job has really been submitted.

[raajeshnew]

thanks , a quick check , when you do the getjobstatus are you checking for null . We get a 1 (waiting to be queued value) on our jobs on getjobstatus but these jobs never get picked up. We will come back once we have tried your fix too

[raajeshnew]

sorry that was for @RobotRobot

[scaleupcto]

Thanks for that @raajeshnew, we'll take a look and amend our solution if necessary. Let me know how you get on.

[scaleupcto]

Seems to me 1 is queued, not waiting to be queued:

class Resque_Job_Status
{
    const STATUS_WAITING = 1;
        ...

    /**
     * Fetch the status for the job being monitored.
     *
     * @return mixed False if the status is not being monitored, otherwise the status as
     *  as an integer, based on the Resque_Job_Status constants.
     */
    public function get()
    {
        if(!$this->isTracking()) {
            return false;
        }

        $statusPacket = json_decode(Resque::redis()->get((string)$this), true);
        if(!$statusPacket) {
            return false;
        }

        return $statusPacket['status'];
    }
}

Surely if the key for the status object was not in redis, then the json_decode fails and the return is false, not 1?

[raajeshnew]

@RobotRobot thanks have you faced a situation where it is 1 but still not getting picked up by threads although they are avl and sleeping

[scaleupcto]

Not thus far. Have you been into redis directly to see if the job is actually queued? Or used the ruby monitoring app https://github.com/resque/resque-web to see if the job is queued?