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

Enable hot-reload via adding ActiveSupport::Reloader delegate #756

Merged
merged 6 commits into from
Feb 5, 2024

Conversation

littleali
Copy link
Contributor

@littleali littleali commented Dec 6, 2023

This PR is related to #720 and it is same as #627 and #725

Pull Request Detail (Copied from #627)

This change adds explicit integration for Rails autoloading. Rails autoloading already partially worked in Shoryuken in that constants that weren't eager loaded were automatically autoloaded. Shoryuken, however, didn't respect config.cache_classes. This meant that once a constant was loaded by the Shoryuken process, it would never be unloaded and reloaded. If you changed the code in a worker class after Shoryuken had loaded it, the change would have no effect until you restarted the Shoryuken process.

The proposed change uses Rails.application.reloader, which takes care of safely reloading constants. This is the same technique employed by active_job and sidekiq and even explicitly documented in the Rails guides. The reloader takes the appropriate locks to ensure that no two threads ever simultaneously unload or reload constants. If the application developer modifies code in an autoloaded directory (including the implementation of workers themselves in app/workers), the changes will take effect the next time a job is processed.

Breaking changes

This change probably warrants a major version bump. The reason for that is that if Shoryuken library code has any reference to objects that belong to autoloaded modules, it's quite easy to run into autoloading errors. If other people's apps are like my own, this is most likely to happen with custom middleware. This is an example from a Rails initializer:

Shoryuken.configure_server do |config|
  config.server_middleware do |chain|
    chain.add MyCustomMiddleware
  end
end

Assume MyCustomMiddleware is defined in an autoloaded directory (basically anything under app, in addition to directories explicitly added to config.autoload_paths), and that the execution of MyCustomMiddleware#call has unqualified references to autoloaded constants. In this case, if Shoryuken tries to invoke that middleware after the application developer has triggered reloading (by modifying files in autoloaded directories), then autoloading will fail with A copy of MyCustomMiddleware has been removed from the module tree but is still active!.

The fix is to ensure that Shoryuken library code never retains a reference to any object defined in autoloaded modules and whose code has references to autoloaded modules. In the example of custom middleware, the easiest solution is to just attach the middleware to workers instead of the global server_middleware. You can even do this in your base worker class.

class ApplicationWorker
  include Shoryuken::Worker

  def self.inherited(klass)
    klass.server_middleware do |chain|
      chain.add MyCustomMiddleware
    end
  end
end

This works because the worker-specific middleware chain has a lifetime associated with the worker class. Once unloading happens, the old references to MyCustomMiddleware are unreachable by the Shoryuken process. When a new job comes in, Shoryuken autoloads the worker class, and the inherited block will run, constructing a new instance of the MyCustomMiddleware class through autoloading.

Users upgrading to a version of Shoryuken that includes this change can, in theory, alternatively just turn on config.cache_classes in their Shoryuken server process (even in development). The experience will be the same as before in that classes won't be reloaded, but it might be an easier upgrade path for some.

Shoryuken.configure_server do
  Rails.application.configure do
    config.cache_classes = true
  end
end

@phstc
Copy link
Collaborator

phstc commented Jan 31, 2024

hi @littleali

Thank you for the PR explanation! This is very helpful.

I was wondering if instead of a major bump, we should enable cache_classes = true or any other way of making reloading optional.

Shoryuken.configure_server do
  Rails.application.configure do
    config.cache_classes = true
  end
end

I believe most people don't define middleware like this

  def self.inherited(klass)
    klass.server_middleware do |chain|
      chain.add MyCustomMiddleware
    end
  end

I'm worried that the change may take some solid debugging time to figure out the issue with the middleware definition.

Any thoughts?

@littleali
Copy link
Contributor Author

Hi @phstc

Regarding your comment I've added a flag called enable_reloading to control reloading. The default value is false which means this non-breaking change provides more control over reloading behaviour.
In addition to the code changes, I've also updated the project's wiki to include documentation on this new flag. This should provide guidance to developers on how and when to use this feature.
Finally, I've updated the existing RSpec tests to ensure this new functionality is covered and works as expected.

@phstc phstc merged commit 7bd25ee into ruby-shoryuken:main Feb 5, 2024
19 checks passed
@phstc
Copy link
Collaborator

phstc commented Feb 5, 2024

@littleali thank you for the explanation and pull request. The new version has been released with your changes. Can you update the wiki?

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.

2 participants