Use after hook

[brucek]

Gets rid of most of the monkeypatching in favor of before and around hooks, as per #7

Note that resque hooks don't give the queue that the job was pulled from, so there's no way to ensure a unique job across multiple queues, but it didn't look like that was in scope as far as I could see from the existing tests.

[teeparham]

Thank you! This looks good. I had a few minor changes.

[teeparham]

Can we write this all as one line?

!ResqueSolo::Queue.queued?(@queue, class: to_s, args: args)

[teeparham]

In all these tests, I prefer to simply hard-code the queue name rather than accessing an internal instance variable. So don't change the tests to use instance_variable_get. There's no reason to change any of these tests except line 47.

[brucek]

Sure, should be good now I think... I haven't yet figured out the workflow to rebase and squash commits yet so let me know if I need to figure out how to make that happen.

[bmckeough]

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.

[brucek]

Yep, you're totally right. I just removed the misleading comment and name.

[bmckeough]

One implication of this change is that a job could be marked enqueued when it wasn't actually enqueued because a different before_enqueue hook returned false.

I'll try to provide a test case to demonstrate, but can express it more quickly in a written example.

Given a job class that includes Resque::Plugins::UniqueJob and also implements a before_enqueue hook that always returns false, before_enqueue_solo_job will mark the job enqueued even though it wasn't actually created.

[brucek]

Auggghh you're right again. It looks like the various Resque hooks actually make it harder to do mark_queued and mark_unqueued properly since failures can happen in many different places, all hooks may not always be called, etc.

Marking queued:

• other before hooks can fail and prevent creating the job, so we can't only mark_queued there
• we have a race condition if we do it in an after_hook

Marking unqueued:

• once the job is dequeued, it may not run due to before_perform hooks returning false, so can't run in an after or around hook
• all before_perform hooks might not even run, ie if any raise Resque::Job::DontPerform or other errors

We can use the before hook to tell if the job has already been queued though. Although there is still a race condition if we don't check again on the actual creation (ie, in the create monkey patch). The right answer using only the Resque hooks is maybe to do some kind of a 2-phase commit and use the key's TTL to expire the lock if another before_hook fails.

I added some monkey patching back in, and now it should be doing a better job marking unqueued after performing the job, so I think this might get rid of the need for the @lock_after_execution_period (since we can call unenqueue immediately after the job and all after_hooks finishes).

Since I moved the mark_enqueued back after job performance, the TTL behavior may no longer be desired, since it doesn't start when the job is dequeued, instead it happens after the job is performed.

This PR is getting kind of messy and I probably should split it up, but I need to find some time for that. If I should remove the @lock_after_execution_period that looks somewhat straightforward, let me know what you think.

Although since last time I thought I knew what I was talking about here and @bmckeough set me straight so I'm going to be a little more careful now ;-) so definitely looking for more feedback here. Thanks guys.