Skip to content

Commit

Permalink
MyWorker.perform_in(1.month) does not always schedule job in one month.
Browse files Browse the repository at this point in the history
At the moment, `MyWorker.perform_in(1.month)` always schedules a job in 30
days. When added to a date, `1.month` will add 1 to the date's month count,
which means that it will add 28, 29, 30, or 31 days depending on the month and
year.
When I call `MyWorker.perform_in(1.month)`, I would expect the job to be
scheduled next month, same day of the month, all the time. At the moment, it is
true only four months in the year.
My pull request tries to fix this by converting the interval object to a Float
at the last possible moment.
Plaese note that the test I wrote will fail only during months that do not have
30 days. Ideally, I would add a dependency to Timecop and freeze time to any
day in a month of 28, 29 or 31 days. This could also avoid using
`#assert_in_delta`, in favour of `#assert_equal`.

Feel free to read my blog post [Rails' `1.month` has a variable
length](http://dstosik.github.io/rails/2015/02/19/rails-1month-variable-length/)
for more details.
  • Loading branch information
davidstosik committed Feb 20, 2015
1 parent f89a0ee commit a2dd36c
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 3 deletions.
6 changes: 3 additions & 3 deletions lib/sidekiq/worker.rb
Expand Up @@ -42,13 +42,13 @@ def perform_async(*args)

def perform_in(interval, *args)
int = interval.to_f
now = Time.now.to_f
ts = (int < 1_000_000_000 ? now + int : int)
now = Time.now
ts = (int < 1_000_000_000 ? (now + interval).to_f : int)

item = { 'class' => self, 'args' => args, 'at' => ts }

# Optimization to enqueue something now that is scheduled to go out now or in the past
item.delete('at'.freeze) if ts <= now
item.delete('at'.freeze) if ts <= now.to_f

client_push(item)
end
Expand Down
9 changes: 9 additions & 0 deletions test/test_scheduling.rb
Expand Up @@ -30,6 +30,15 @@ def perform(x)
@redis.verify
end

it 'schedules a job in one month' do
@redis.expect :zadd, true do |key, args|
assert_equal 'schedule', key
assert_in_delta 1.month.since.to_f, args[0][0].to_f, 1
end
assert ScheduledWorker.perform_in(1.month, 'mike')
@redis.verify
end

it 'schedules a job via timestamp' do
@redis.expect :zadd, true, ['schedule', Array]
assert ScheduledWorker.perform_in(5.days.from_now, 'mike')
Expand Down

0 comments on commit a2dd36c

Please sign in to comment.