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

Do not unlock on sidekiq shutdown #87

Merged
merged 6 commits into from
Jun 22, 2015
Merged

Conversation

deltaroe
Copy link
Contributor

somewhat related to #77

The current behavior of sidekiq-unique-jobs is to unlock a job when job is done regardless of if the job succeeds or fails. From what I can tell this is desired because sidekiq requeues failed jobs and sidekiq-unique-jobs would otherwise prevent them from being queued.

There is one exception and that is when a task is killed due to exceeding a timeout during a worker shutdown. https://github.com/mperham/sidekiq/blob/master/lib/sidekiq/processor.rb#L55-L59 In that case the job is left and the worker picks it back up when it restarts. The task kill generates an exception and sidekiq-unique-job removes the lock even though the job is still there. When the worker starts again it picks up the job again, but non-unique work can be queued because the lock was already removed.

This PR duplicates the logic of the sidekiq processor to also catch the shutdown exception and not unlock the job in that case.

I first noticed this when having a unique reoccurring task that took longer than the reoccurrence (ran for 100 seconds, but started every minute) If the worker was restarted while the task was running it would start up again and at the top of the next minute a duplicate task would start. When the first task would complete it would unlock the lock placed by the second task and the two jobs would leapfrog forever.

mhenrixon added a commit that referenced this pull request Jun 22, 2015
Do not unlock on sidekiq shutdown
@mhenrixon mhenrixon merged commit 4843b89 into mhenrixon:master Jun 22, 2015
@mhenrixon
Copy link
Owner

Looks great! Thanks for this

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

Successfully merging this pull request may close these issues.

2 participants