mhenrixon / sidekiq-unique-jobs

Prevents duplicate Sidekiq jobs
MIT License
1.47k stars 278 forks source link

Short jobs are not unique for the given time window #33

Closed mmmries closed 10 years ago

mmmries commented 10 years ago

I ran into a bug at work where the same job was being run multiple times (sending out emails to users). So I did some testing and came up with a minimal reproduction of the problem here: https://github.com/hqmq/sidekiq-not-unique-jobs

In order to run it just bundle install and then in one terminal start sidekiq like normal:

$ bundle exec sidekiq -r config/bootstrap.rb

Then in a second terminal run:

$ ruby test_uniqueness.rb
2014-02-26T21:50:12Z 67772 TID-znnu9k INFO: Sidekiq client with redis options {:url=>"redis://localhost:6379", :namespace=>"sk"}
Expecting the counter to = 1
counter = 9
mmmries commented 10 years ago

The test basically just sets a key to 0, then attempts to run the job 100 times (in ten separate threads to simulate multiple processes/servers), then it sleeps for a second to allow the background job(s) to run and checks the value in the counter.

As you can see multiple jobs end up getting run (9 in the case above) and the counter gets incremented more than once.

I think this is caused by the fact that the uniqueness key is being cleared when the job is completed, but this means that the uniqueness window is closed immediately rather than letting it expire after the desired amount of time.

Am I misunderstanding how this gem is supposed to work or is this a bug?

mmmries commented 10 years ago

I just updated my test to be a bit more thorough. I gave the worker a :unique_job_expiration => 10 and changed my test to try to queue up 100 workers, wait 3 seconds try to queue another 100 and finally wait 10 seconds and try to queue another 100 workers. So the final expected counter value would be 2, but I get this:

$ ruby test_uniqueness.rb
2014-02-26T22:35:07Z 70948 TID-znnu9k INFO: Sidekiq client with redis options {:url=>"redis://localhost:6379", :namespace=>"sk"}
Expecting counter = 1
counter = 7
Expecting counter = 1
counter = 16
Expecting counter = 2
counter = 25

I also confirmed that removing the unlock from lib/sidekiq-unique-jobs/middleware/server/unique_jobs.rb fixes this and makes the test run correctly.

mhenrixon commented 10 years ago

Perhaps this could be improved since people do seem to have some trouble with figuring it out. First of all you need to add the sidekiq-unique-jobs gem. Secondly

If you change your worker to

class Incrementer
  include Sidekiq::Worker
  sidekiq_options :unique => true, :unique_job_expiration => 10, :unique_unlock_order => :never

  def perform
    Sidekiq.redis do |conn|
      conn.incr :counter
    end
  end
end

it should work like you expected where the uniqueness window is kept even though the job has been successfully processed. I created a pull request for you hqmq/sidekiq-not-unique-jobs#1