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

Fix backport of before_pause & after_pause hooks #1334

Merged
merged 2 commits into from
Nov 11, 2015
Merged

Fix backport of before_pause & after_pause hooks #1334

merged 2 commits into from
Nov 11, 2015

Conversation

stevenschobert
Copy link
Contributor

Heya awesome folks! Thank you for all awesome work that goes into resque 🎉

I ran into a small issue today, in the 1-x-stable version (v1.25.2), where before_pause and after_pause hooks would never get called. Workers are still paused correctly, they just don't fire their associated hooks.

After a bit of digging, I think there was just a small mixup in #831 when the hooks got backported to 1-x-stable (which is awesome by the way, ❤️ for the backport).

I've added in the run_hook calls to the pause_processing and unpause_processing methods respectively, and added specs to cover the change.

Cheers! Please let me know if there's anything else I can do!

when this was backported to 1.x-stable in
14cb8a9, the wrong
test name was copied in
@courtsimas
Copy link

Yep, I +1 this.

@hoffmanc
Copy link
Contributor

hoffmanc commented Nov 5, 2015

Hey there! It's been a while, sorry about that.

@steveklabnik and I are going to be working on Resque again, but this PR needs some attention. If you get the chance, would you mind

making the tests pass, if you're up for it.

If you're not up for it, that's cool, just let us know so we can investigate
Thanks! / sorry 😦

@stevenschobert
Copy link
Contributor Author

No worries on the wait, I totally understand!

I'm totally happy to help investigate the specs failing. It likes like the specs are all passing except on ruby 1.9.2, with the following error:

  1) Failure:
#<Class:0x000000017d9438>#test_SIGTERM_and_cleanup_does_not_occur_in_allotted_time [/home/travis/build/resque/resque/test/child_killing_test.rb:25]:
<nil> expected to not be nil.

  2) Failure:
#<Class:0x000000017d9438>#test_SIGTERM_and_cleanup_occurs_in_allotted_time [/home/travis/build/resque/resque/test/child_killing_test.rb:25]:
<nil> expected to not be nil.

  3) Failure:
#<Class:0x000000017d9438>#test_old_signal_handling_just_kills_off_the_child [/home/travis/build/resque/resque/test/child_killing_test.rb:25]:
<nil> expected to not be nil.

After digging through some other pull-requests, I noticed that #1343 also failed for the same reason. Any ideas as to what the root of this issue might be? Could something in this changeset have introduced that error?

@hoffmanc
Copy link
Contributor

hoffmanc commented Nov 5, 2015

Hi @stevenschobert; good question. I closed out that PR because I didn't realize that there were separate gemfiles for 1.8.7 and 1.9.2. Once I get that cleared up I'll rerun and see what the output looks like.

@hoffmanc
Copy link
Contributor

hoffmanc commented Nov 5, 2015

Looks like these failures are not deterministic... which makes sense, since they rely on forking another process. I'm unable to repro locally right now, but will keep trying.

@hoffmanc
Copy link
Contributor

@steveklabnik 👍

steveklabnik added a commit that referenced this pull request Nov 11, 2015
Fix backport of before_pause & after_pause hooks
@steveklabnik steveklabnik merged commit f522a5f into resque:1-x-stable Nov 11, 2015
@steveklabnik
Copy link
Member

Thanks so much for this!

@stevenschobert stevenschobert deleted the pause-hooks branch November 11, 2015 15:32
@stevenschobert
Copy link
Contributor Author

@steveklabnik @hoffmanc Thank you both! Happy I was able to help!

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.

4 participants