Navigation Menu

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

Remove delayed extensions #5076

Closed
Tracked by #5080
mperham opened this issue Dec 1, 2021 · 10 comments
Closed
Tracked by #5080

Remove delayed extensions #5076

mperham opened this issue Dec 1, 2021 · 10 comments

Comments

@mperham
Copy link
Collaborator

mperham commented Dec 1, 2021

They have been disabled by default since Sidekiq 5.0 and per #5064, Ruby 3.1 is proposing to flag the YAML load operation as unsafe. Remove them from core.

I fully expect and am fine with someone baking the code into a 3rd party gem which people can use if they wish to continue using the delayed extensions.

@mperham
Copy link
Collaborator Author

mperham commented Dec 1, 2021

https://github.com/mperham/sidekiq/wiki/Delayed-extensions

@bf4
Copy link
Contributor

bf4 commented Jan 21, 2022

We rely heavily on the delayed class extensions as a pattern to easily run any function async.
(I've have mixed feelings about it since I know what it does under the hood with the YAML un/marshalling, but it sure is convenient.)

I expect I won't be the only person looking for some discussion of an upgrade path, so I thought I'd mention it here.

I think for the most part we should be able to change

class User
  def self.call_me(*args, **options)
    #
  end

  def self.call_me_async(*args, **options)
    delay_for(1.minute, retry: 0, queue: :default).call_me(*args, **options)
  end

  def call_me_async
    self.class.call_me_async
  end
end

to use a regular worker with something like

class GenericWorker < ApplicationWorker

  def _perform(klass_name, *args)
    klass = klass_name.constantize
    options = extract_options!(args)
    if options
      logger.info "[#{jid}] GenericWorker executing #{klass_name}.public_send(*#{args.inspect}, **#{options.inspect})"
      klass.public_send(*args, **options)
    else
      logger.info "[#{jid}] GenericWorker executing #{klass_name}.public_send(*#{args.inspect})"
      klass.public_send(*args)
    end
  end
end

and changing the User code to

    def self.call_me_async(*args, **options)
-     delay_for(1.minute, retry: 0, queue: :default).call_me(*args, **options)
+     GenericWorker.set(retry: 0, queue: :default).perform_in(1.minute, model_name.name, :call_me, *args, **options)
    end

and our test helpers would change something like

  def sidekiq_delayed_jobs
    sidekiq_worker_class.jobs
  end

  def sidekiq_worker_class
-    Sidekiq::Extensions::DelayedClass
+    GenericWorker
  end

  def sidekiq_job_attributes(sidekiq_job)
-    # @note Yes, this is unsafe
-    YAML.unsafe_load(sidekiq_job["args"].first)
+    JSON.parse(sidekiq_job["args"].first)
  end

@mperham
Copy link
Collaborator Author

mperham commented Jan 21, 2022

I have no problem with the delayed extensions going into a 3rd party gem which maintains the exact same API. I just don’t want them in core anymore. This could be as simple as:

gem “sidekiq-delay”

Or whatever.

@wxmn
Copy link

wxmn commented Feb 2, 2022

@bf4 please let me know if and when "sidekiq-delay" becomes a reality 😄

I like the idea of getting rid of YAML too 🤔

@bf4
Copy link
Contributor

bf4 commented Feb 2, 2022

I'm willing to do the work to extract the delayed extensions into a gem, but I'm not really interested in maintaining it, so not sure what good that does anyone.

@bf4
Copy link
Contributor

bf4 commented Feb 8, 2022

I took a stab at extracting the delayed extensions in gemhome/sidekiq-delay_extensions#1

Got one failing test, but most of the changes are bulk code wrangling. I haven't reviewed the full changeset at this time for correctness.

My intention would be to release in lockstep with sidekiq until 7.0, at which point it would possibly begin to diverge

If it works out, I'll push it to rubygems. PRS welcome

@bf4
Copy link
Contributor

bf4 commented Feb 8, 2022

Got tests passing. And happy enough with it to release as https://rubygems.org/gems/sidekiq-delay_extensions
Only 6.4.0 and 6.4.1 right now

@swrobel
Copy link

swrobel commented Jun 28, 2022

What's the new recommended way to interact with ActionMailer? The only place it's referenced in the wiki is showing how to use it with delayed extensions.

@mperham
Copy link
Collaborator Author

mperham commented Jun 28, 2022

The idiomatic Rails way is deliver_later.

@swrobel
Copy link

swrobel commented Jun 28, 2022

The idiomatic Rails way is deliver_later.

Thanks. I know you're not a big fan of ActiveJob so I wanted to see if there was some other method that you recommended instead.

@mperham mperham closed this as completed Sep 9, 2022
vasconsaurus added a commit to meedan/pender that referenced this issue Nov 23, 2023
- Delayed extensions has been disabled since sidekiq 5, and removed from 7
sidekiq/sidekiq#5076
So I replaced it with a new job.

- I also called it a job instead of worker as per Sidekiq's documentation
https://github.com/sidekiq/sidekiq/wiki/Best-Practices#4-use-precise-terminology
https://stackoverflow.com/questions/76095172/sidekiq-worker-vs-job
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 a pull request may close this issue.

4 participants