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

Use after hook #10

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ gem 'resque_solo'
class UpdateCat
include Resque::Plugins::UniqueJob
@queue = :cats
@lock_after_execution_period = 20

def self.perform(cat_id)
# do something
Expand Down
27 changes: 0 additions & 27 deletions lib/resque_ext/job.rb
Original file line number Diff line number Diff line change
@@ -1,39 +1,12 @@
module Resque
class Job
class << self
# Mark an item as queued
def create_solo(queue, klass, *args)
item = { class: klass.to_s, args: args }
if Resque.inline? || !ResqueSolo::Queue.is_unique?(item)
return create_without_solo(queue, klass, *args)
end
return "EXISTED" if ResqueSolo::Queue.queued?(queue, item)
create_return_value = false
# redis transaction block
Resque.redis.multi do
create_return_value = create_without_solo(queue, klass, *args)
ResqueSolo::Queue.mark_queued(queue, item)
end
create_return_value
end

# Mark an item as unqueued
def reserve_solo(queue)
item = reserve_without_solo(queue)
ResqueSolo::Queue.mark_unqueued(queue, item) if item && !Resque.inline?
item
end

# Mark destroyed jobs as unqueued
def destroy_solo(queue, klass, *args)
ResqueSolo::Queue.destroy(queue, klass, *args) unless Resque.inline?
destroy_without_solo(queue, klass, *args)
end

alias_method :create_without_solo, :create
alias_method :create, :create_solo
alias_method :reserve_without_solo, :reserve
alias_method :reserve, :reserve_solo
alias_method :destroy_without_solo, :destroy
alias_method :destroy, :destroy_solo
end
Expand Down
17 changes: 0 additions & 17 deletions lib/resque_ext/resque.rb
Original file line number Diff line number Diff line change
@@ -1,22 +1,5 @@
module Resque
class << self
def enqueue_to(queue, klass, *args)
# Perform before_enqueue hooks. Don't perform enqueue if any hook returns false
before_hooks = Plugin.before_enqueue_hooks(klass).collect do |hook|
klass.send(hook, *args)
end
return nil if before_hooks.any? { |result| result == false }

result = Job.create(queue, klass, *args)
return nil if result == "EXISTED"

Plugin.after_enqueue_hooks(klass).each do |hook|
klass.send(hook, *args)
end

true
end

def enqueued?(klass, *args)
enqueued_in?(queue_from_class(klass), klass, *args)
end
Expand Down
5 changes: 3 additions & 2 deletions lib/resque_solo/queue.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ def queued?(queue, item)
end

def mark_queued(queue, item)
return unless is_unique?(item)
key = unique_key(queue, item)
redis.set(key, 1)
res = redis.setnx(key, 1)
return false unless res
ttl = item_ttl(item)
redis.expire(key, ttl) if ttl >= 0
res
end

def mark_unqueued(queue, job)
Expand Down
15 changes: 15 additions & 0 deletions lib/resque_solo/unique_job.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,21 @@ def ttl
def lock_after_execution_period
@lock_after_execution_period ||= 0
end

# We want this to run first in before_enqueue_hooks (which are alpha sorted), so name appropriately
def before_enqueue_001_solo_job(*args)

Choose a reason for hiding this comment

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

Really excited to find resque_solo, the unreleased changes to make it not call after_enqueue_hooks if the job was not created, and this PR which makes the implementation hook based.

Why does the order of execution of before_enqueue hooks matter?

All before_enqueue_hooks will be invoked by Resque.enqueue_to, regardless of the result of any individual hook. So, it isn't clear to me why the order the hooks are invoked would matter.

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're totally right. I just removed the misleading comment and name.

# This returns false if the key was already set
ResqueSolo::Queue.mark_queued(@queue, { class: self.to_s, args: args })
end

# Always marks unqueued, even on failure
def around_perform_solo_job(*args)
begin
yield
ensure
ResqueSolo::Queue.mark_unqueued(@queue, { class: self.to_s, args: args })
end
end
end
end
end
Expand Down
2 changes: 2 additions & 0 deletions resque_solo.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,7 @@ Gem::Specification.new do |spec|
spec.add_development_dependency "bundler", "~> 1.6"
spec.add_development_dependency "fakeredis", "~> 0.4"
spec.add_development_dependency "minitest", "~> 5.8"
spec.add_development_dependency "minitest-reporters", "~> 1.1"
spec.add_development_dependency "rake", "~> 11.2"
spec.add_development_dependency "m", "~> 1.5"
end
7 changes: 3 additions & 4 deletions test/job_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ class JobTest < MiniTest::Spec
Resque.enqueue FakeUniqueJob, "foo"
Resque.enqueue FakeUniqueJob, "foo"
assert_equal 1, Resque.size(:unique)
Resque.reserve(:unique)
perform_one_manually(:unique)
assert_equal 0, Resque.size(:unique)
Resque.enqueue FakeUniqueJob, "foo"
Resque.enqueue FakeUniqueJob, "foo"
Expand Down Expand Up @@ -48,8 +48,7 @@ class JobTest < MiniTest::Spec
it "mark jobs as unqueued when they raise an exception" do
2.times { Resque.enqueue(FailingUniqueJob, "foo") }
assert_equal 1, Resque.size(:unique)
worker = Resque::Worker.new(:unique)
worker.work 0
assert_raises { perform_one_manually(:unique) }
assert_equal 0, Resque.size(:unique)
2.times { Resque.enqueue(FailingUniqueJob, "foo") }
assert_equal 1, Resque.size(:unique)
Expand Down Expand Up @@ -98,7 +97,7 @@ class JobTest < MiniTest::Spec

it "honor lock_after_execution_period in the redis key" do
Resque.enqueue UniqueJobWithLock
Resque.reserve(:unique_with_lock)
perform_one_manually(:unique_with_lock)
keys = Resque.redis.keys "solo:queue:unique_with_lock:job:*"
assert_equal 1, keys.length
assert_in_delta UniqueJobWithLock.lock_after_execution_period,
Expand Down
11 changes: 6 additions & 5 deletions test/resque_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,19 +26,20 @@ class ResqueTest < MiniTest::Spec
describe "#enqueue_to" do
describe "non-unique job" do
it "should return true if job was enqueued" do
assert Resque.enqueue_to(:normal, FakeJob)
assert Resque.enqueue_to(:normal, FakeJob)
assert Resque.enqueue_to(:unique, FakeJob)
assert Resque.enqueue_to(:unique, FakeJob)
end
end

describe "unique job" do
it "should return true if job was enqueued" do
assert Resque.enqueue_to(:normal, FakeUniqueJob)
assert Resque.enqueue_to(:unique, FakeUniqueJob)
end

it "should return nil if job already existed" do
Resque.enqueue_to(:normal, FakeUniqueJob)
assert_nil Resque.enqueue_to(:normal, FakeUniqueJob)
Resque.enqueue_to(:unique, FakeUniqueJob)
assert Resque.enqueued?(FakeUniqueJob)
assert_nil Resque.enqueue_to(:unique, FakeUniqueJob)
end
end
end
Expand Down
9 changes: 8 additions & 1 deletion test/test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,18 @@
end

require "minitest/autorun"
require "minitest/reporters"
require "resque_solo"
require "fake_jobs"
require "fakeredis"
require "fakeredis/minitest"
begin
require "pry-byebug"
rescue LoadError
# ignore
end

Minitest::Reporters.use! [Minitest::Reporters::DefaultReporter.new({ color: true })]

def perform_one_manually(queue_name)
Resque::Job.reserve(queue_name).perform
end