Unblock rufus-scheduler upgrade by fixing shutdown #736
Conversation
carsonreinke
changed the title
|
Thanks @carsonreinke! This would be really good to get merged, thanks in advance 🙇 cc @iloveitaly |
|
@iloveitaly I have confirmed those changes get tests passing and allowing dependencies to get upgraded. Is there anything else pending to get this PR merged? Keep Rocking! |
|
@fagiani I've upgraded CI to do a matrix test across a bunch of rufus & resque versions. Can you rebase on master to fix merge conflicts and ensure this is tested across different configurations? |
|
@iloveitaly I've tried to set myself as a reviewer in order to check if I could push the rebased branch but as far as I was able to find, in order to achieve that the PR must allow changes from maintainers and I need to become a contributor somehow. In respect to @carsonreinke's work I did not recreate a new PR. Please let me know how would you like to proceed. |
carsonreinke
force-pushed
the
rufus-scheduler-unblock
branch
from
68732ae to
3b27fe3
Compare
|
@iloveitaly @fagiani I pushed up a rebase, let me know if you need anything else. |
| @@ -377,7 +377,6 @@ def handle_signals_with_operation | |||
|
|
|||
| def stop_rufus_scheduler | |||
| rufus_scheduler.shutdown(:wait) | |||
| rufus_scheduler.join | |||
There was a problem hiding this comment.
Looking at the CI tests, looks like the only version failing is 3.6.
I'd suggest something like:
rufus_scheduler.join if Gem.loaded_specs["rufus_scheduler"].version.to_s.starts_with?("3.6")I'd guess this change would potentially fix the test.
There was a problem hiding this comment.
@fagiani ugh, looks like a flake, rebuilt and it passed. Ran it locally without failure. I don't believe that failure would even be related to this change though.
There was a problem hiding this comment.
|
Thanks for merging this, @iloveitaly! Any chance we could get this released? Thanks in advance |
Updates #723
Looking into this more, it appears the use of calling
#joinon the scheduler is not needed when calling#shutdown(:wait)since that will block the current thread until shutdown. The reason this worked before looks like simplyrufus-schedulerunexpectedly supported this and it appears completely accidental. Looking into the differences it looks the reason why is that the oldrufus-schedulermethod allowed calls on#joinafter shutdown because the@threadvariable is never reset after shutdown (see https://github.com/jmettraux/rufus-scheduler/blob/v3.2.0/lib/rufus/scheduler.rb#L171). After changes in v3.6.0, this is no longer allowed since it is using a queue system instead.