Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #173 #244

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

askobara
Copy link

@askobara askobara commented Jun 13, 2018

What about the solution such this? It's not finished yet, just to pic the idea of using yii/redis/Mutex to solve the concurrency issue.

Q A
Is bugfix? yes
New feature? no
Breaks BC? no
Tests pass? yes
Fixed issues #173

@samdark
Copy link
Member

samdark commented Jun 13, 2018

The idea itself sounds good to me.

@askobara askobara force-pushed the bugfix/use-redis-mutex-to-lock-queue branch from 5cc180d to f0df034 Compare June 13, 2018 13:33
@dmirogin dmirogin requested a review from zhuravljov June 13, 2018 13:35
} else {
// job is failed but we want to return it back into
// the queue
$this->redis->zadd("$this->channel.reserved", time(), $id);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That cold lead to fail-add indefinite cycle. Should be limited number of re-adding the job back.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you are right, but I rely on a class/global defined number of attempts. In this case if all attempts have been used up, job would be marked as a handled and this condition will never be invoked.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha.

@samdark
Copy link
Member

samdark commented Jun 18, 2018

@zhuravljov what do you think?

@samdark samdark added the type:bug Bug label Jun 18, 2018
@askobara askobara changed the title [WIP] fix #173 fix #173 Jun 22, 2018
@askobara
Copy link
Author

Also, a few things are confusing me a bit in this PR:

  1. Method reserve doesn't make a reserve actually.
  2. Placing a job's id into "reserved" queue are making from two parts of the class.

Should I fix it?

@askobara
Copy link
Author

Any updates?
I'm already using this patch in production, and seems like it's works.

@rob006
Copy link

rob006 commented Jul 22, 2018

https://github.com/yiisoft/yii2-redis/blob/master/src/Mutex.php#L113-L119

If lock cannot be set, redis mutex will try again after 1 second. 1 second is like ages in this context - I'm able to handle tens of jobs in this time.

@askobara
Copy link
Author

Implementation of the mutex can be changed via config. It's only default behavior.

@rob006
Copy link

rob006 commented Jul 22, 2018

Right now instance of yii\redis\Mutex is required. Also remove() and clear() uses higher frequency for mutex check - why reserving job should be slower?

@askobara
Copy link
Author

Yeah, right now yii\redis\Mutex is using, but no one forbid to inherit some class from it and use this implementation in the queue. Yii provides yii\redis\Mutex with 1 second delay, by default. I guess, this issue is not about the PR.

However, you are right about the unfairly low delay of other actions. I could suggest to try to use same mutex for those actions and also, start a discussion in the main repo about decreasing delay of yii\redis\Mutex.

What do you think?

@rob006
Copy link

rob006 commented Jul 22, 2018

Yeah, right now yii\redis\Mutex is using, but no one forbid to inherit some class from it and use this implementation in the queue.

But there is no valid reason for requiring yii\redis\Mutex, any mutex component will work fine.

@askobara
Copy link
Author

Oh, I get it.

@cebe
Copy link
Member

cebe commented Aug 8, 2018

https://github.com/yiisoft/yii2-redis/blob/master/src/Mutex.php#L113-L119

If lock cannot be set, redis mutex will try again after 1 second. 1 second is like ages in this context - I'm able to handle tens of jobs in this time.

would be a good idea to make the timeout a float in redis mutex to allow retry much faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants