-
Notifications
You must be signed in to change notification settings - Fork 20
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
Heads up: resque-scheduler 4.9+ on redis-rb 4.x inadvertently breaks loner locks #36
Comments
Do you happen to have a fork with a mitigation? I just started looking at this issue, as we just ran into it, and looking for any prior art before I go spelunking. |
No forks I'm afraid, but the mitigations I see:
|
Thanks, yep. I'm currently rolling back scheduler & redis to mitigate a live issue now, and will be looking into option 2. Option 3 I think would require 'injecting' Resque.enqueue with a given pipeline'd instance, which looks... not easy right now but maybe a longer term fix. FWIW, I'll probably publish both that and our fork of resque-lock-timeout (which incorporates a few of the other fork fixes that have been around). |
Nice. Hugops and will be curious to see to any other tweaks you have lying around. :) Now that I'm spooling context back up on option 3 (credit to this comment), I think even that refactor still wouldn't resolve resque/resque-scheduler#779 since enqueue hooks would still be executed in the middle of an open transaction so...option 2 seems like honestly the most feasible thing anyway. |
Was just able to come back and finish up some work around this. Ibotta/resque-scheduler#1 This PR has my proposal (currently a PR against my own fork). It allows handling the delayed enqueue batching as an opt-in option. As noted in the comments, I'm open to switching it to be opt-out (which we'd use). I've also linked my integration test Ibotta/resque-plugin-integration-tests#1 between I'm going to let this bake a bit, and do some experimentation in our internal repositories, but I'll likely propose the patch 'upstream' as well. |
Due to resque/resque-scheduler#767, resque-scheduler 4.9+ now wraps all enqueues within a Redis transaction. This means that Redis commands in
before_enqueue
hooks (inadvertently) no longer run synchronously under redis 4.x, which breaks gems like resque-lock-timeout (since it usesbefore_enqueue
hooks for loner lock checks).Repro:
This seems to not be an issue on Redis 5.x, although there are other problems with resque-scheduler's transactions there, since some commands inadvertently "spill out" of the ongoing transaction (cf. resque/resque-scheduler#787).
I think this is more a bug for resque-scheduler to fix (specifically resque/resque-scheduler#779), but it affects this gem. So, heads up.
Further reading:
The text was updated successfully, but these errors were encountered: