Optional extension loading

[hundredwatt]

Relates to #1684

The developer can load Sidekiq without the extensions injected by requiring sidekiq/no_extensions instead of sidekiq.

[hundredwatt]

I tested this by running the following commands in the Sidekiq repository:

1. With extensions, tests pass

bundle exec ruby -Ilib -Itest -rsidekiq -rsidekiq/api test/test_extensions.rb

2. Without extensions, tests fail

bundle exec ruby -Ilib -Itest -rsidekiq/no_extensions -rsidekiq/api test/test_extensions.rb

[mperham]

I like this but I'd prefer a global method to be called, e.g. Sidekiq.disable_extensions! instead of a special require.

[hundredwatt]

@mperham I did some experimenting with a global method or as a configuration option, but there's definitely some friction. Consider the following, simplified example extracted from test/test_extensions.rb:

require 'helper'
require 'sidekiq' # Mark 1
require 'active_record'
require 'action_mailer'
require 'sidekiq/extensions/action_mailer'
require 'sidekiq/extensions/active_record'
require 'sidekiq/rails'

Sidekiq.hook_rails! # This would be impliciltly called in a real Rails app

# Mark 2

class SomeClass
  def self.doit(arg)
  end
end

SomeClass.delay.doit(Date.today) # Mark 3

Ideally, we'd like to be able to put a call to Sidekiq.disable_extensions! at `Mark 2 which would then lead to a NoMethodError exception at Mark 3.

Since no other Sidekiq code executes between Mark 2 and Mark 3, I see a limited number of options for how Sidekiq.disable_extensions! could work.

The simpliest, but potential very impactful, option I tried involved overriding Object#method_missing and then when SomeClass.delay is called, check if extensions are enabled. If they are, include them using the Sidekiq.run_extension_load_hooks method from this PR.
Here's the implementation: https://github.com/hundredwatt/sidekiq/commit/17d134b577121ceabea48f192e6f67688afecceb

One problem is the tests won't pass if run together. To get it to work, the two disabling extensions related tests must be run in a separate process since there's interaction with what code is loaded.

A couple other options for consideration:

1. Use meta-programming to undefine all the extension methods individually. This could be really messing and cause problems if other ancestors define, eg: #delay
2. When SomeClass.delay is called, check to see if extensions are enabled and then do the right thing. This is also complicated and my gut feel is that there are a number of edge-cases not covered. Here's an (untested) sample implementation:

      def delay(*args)
        if Sidekiq.extensions_disabled?
          if defined?(super)
            super(*args)
          else
            raise NoMethodError.new("undefined method `delay' for #{self.inspect}:#{self.class}")
          end
        else
          sidekiq_delay(*args)
        end
      end

(This could easily be extracted into its own macro to replace alias_method)

Any suggestions or feedback?

[mperham]

The problem is (2) is that people don't want #delay registered as a method in the first place. Someone else offered (1) a few months ago; I declined it because it was messy.

Could we register the delay methods in a :startup event which is executed after Rails initializers are run? An initializer could call config.disable_delay! and the event block would do nothing.

[hundredwatt]

@mperham What about https://github.com/hundredwatt/sidekiq/commit/17d134b577121ceabea48f192e6f67688afecceb, specifically: https://github.com/hundredwatt/sidekiq/blob/17d134b577121ceabea48f192e6f67688afecceb/lib/sidekiq/extensions/object.rb?

I decided to put the Sidekiq.run_extension_load_hooks! in Object#method_missing so it would work outside of Rails. This could easily be adapted to the approach you described with a startup event that includes the extensions as long as disable! hasn't been called.

Seems like the ideal approach might be:

For Rails applications: use your approach

For non-Rails applications: Don't load the extensions by default

Other than that, I don't see a way to have the extensions load by default AND be disable-able without meta-programming / doing what I did in my example.

[mperham]

I think that's overly complicated. If we just activate delay in the startup hook, it should be really clean. Non-rails apps are using the -r option so that required file can disable the extensions when the file is required.

[mperham]

Shoot, I knew I was forgetting something. #delay is relevant in the client and there's no startup hook in the client.

[mperham]

I'm wondering if the right thing is to make #delay opt-in in Sidekiq 4.0.

[hundredwatt]

An initializer approach for Rails would still work for the client. For non Rails app, we could load the extensions immediately. That way, we wouldn't break the 3.x API. Something like:

require 'sidekiq/extensions/class_methods'
require 'sidekiq/extensions/action_mailer'
require 'sidekiq/extensions/active_record'
if defined?(::Rails::Engine)
  require 'sidekiq/rails' 
else
  # immediately load extensions
end

In general, I agree with @dhh's comment that the #delay method makes it hard to understand what's happening in the background queue. We've certainly experienced that first hand in our apps. There are a few specific cases where it may make sense to allow delaying without an explicit worker. One I was a big fan of was ActionMailer.async (which was removed when Rails.queue was dropped). In the case of mailers, MyMailer.send_something is substantially simpler than create a new worker class to send mailers. So, perhaps it'd be better to offer #delay as a mixin, rather than including it globally. Is this what you're thinking behind your last comment?

Should I look into the above approach for now?

[mperham]

I like it. Go for it and let's see what you come up with.

[hundredwatt]

Ok, will do next week

[mperham]

Just realized there's Sidekiq.remove_delay!

[hundredwatt]

I made this little sample to test that approach in Ruby 2.0. Seems to work fine and doesn't interfere with other inherited modules:

class Base
  #def bar
  #  p "Base's method"
  #end
end

module A
  def bar
    p "A's method"
  end
end

module B
  module Ext
    def foo
      p "B's method"
    end
    alias :bar :foo
  end
end

Base.send(:include, A)
Base.send(:include, B::Ext)

Base.new.bar #=> "B's method"

B::Ext.module_eval do
  remove_method :bar
end

Base.new.bar #=> "A's method"

[hundredwatt]

@mperham leveraging remove_delay!, this commit patches extension loading so we can prevent the extensions from ever being loaded in a Rails app: https://github.com/hundredwatt/sidekiq/commit/8cf0bf447c690e5681f4b25efa9dc17559f7968a

That would complete the approach we described above. Thoughts?

[mperham]

Looks pretty slick. Would you update the Delay Extensions wiki page with a blurb on this?

[hundredwatt]

Thought of an edge case, am going to fix it and then I will update the wiki

[hundredwatt]

Nevermind, edge case is actually a benefit, will describe in wiki

[hundredwatt]

@mperham wiki updated: https://github.com/mperham/sidekiq/wiki/Delayed-extensions. Thanks for your feedback, this was an interesting exploration!