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

Rails 7: issues reloading a class that uses delegate_missing_to and references other class #44125

Closed
machty opened this issue Jan 8, 2022 · 5 comments

Comments

@machty
Copy link
Contributor

machty commented Jan 8, 2022

Rails Master, Ruby 3.1.0

In my app, I have app/models/test_inventory.rb defined as follows:

module TestInventory
  class << self
    delegate_missing_to :catalog

    def catalog
      Item

      # ...omitted code that reads from a YAML and builds a singleton
      # "catalog" from items in the yaml; ultimately the issue stems
      # from referencing the Item class
    end
  end
end

The first boot / render works fine, but once I make a code change and zeitwerk tries to reload, I hit the following error:

uninitialized constant #<Class:TestInventory>::Item
app/models/test_inventory.rb:12:in `catalog'
/Users/machty/code/ruby-on-rails/activesupport/lib/active_support/core_ext/module/delegation.rb:299:in `respond_to_missing?'
/Users/machty/code/ruby-on-rails/railties/lib/rails/application/finisher.rb:40:in `respond_to?'
/Users/machty/code/ruby-on-rails/railties/lib/rails/application/finisher.rb:40:in `block (2 levels) in <module:Finisher>'
zeitwerk (2.5.3) lib/zeitwerk/loader.rb:489:in `block in run_on_unload_callbacks'
zeitwerk (2.5.3) lib/zeitwerk/loader.rb:489:in `each'
zeitwerk (2.5.3) lib/zeitwerk/loader.rb:489:in `run_on_unload_callbacks'
zeitwerk (2.5.3) lib/zeitwerk/loader.rb:149:in `block (2 levels) in unload'
zeitwerk (2.5.3) lib/zeitwerk/loader.rb:146:in `each'
zeitwerk (2.5.3) lib/zeitwerk/loader.rb:146:in `block in unload'
zeitwerk (2.5.3) lib/zeitwerk/loader.rb:125:in `synchronize'
zeitwerk (2.5.3) lib/zeitwerk/loader.rb:125:in `unload'
zeitwerk (2.5.3) lib/zeitwerk/loader.rb:194:in `reload'
/Users/machty/code/ruby-on-rails/activesupport/lib/active_support/dependencies.rb:79:in `block in clear'
/Users/machty/code/ruby-on-rails/activesupport/lib/active_support/concurrency/share_lock.rb:151:in `exclusive'
/Users/machty/code/ruby-on-rails/activesupport/lib/active_support/dependencies/interlock.rb:17:in `unloading'
/Users/machty/code/ruby-on-rails/activesupport/lib/active_support/dependencies.rb:32:in `unload_interlock'
/Users/machty/code/ruby-on-rails/activesupport/lib/active_support/dependencies.rb:77:in `clear'

I believe what's happening is that:

  1. When zeitwerk reloads, it happens to unload the Item class prior to unloading TestInventory
  2. When it gets to on_unload for the TestCatalog class, it hits the value.respond_to?(:before_remove_const)
  3. this invokes the respond_to_missing? logic provided by delegate_missing_to, which ultimately calls respond_to? on the delegated-to object, which calls the catalog method, which references Item, which has been unloaded

FYI: this is working for us on Rails 6.1; perhaps some ordering thing changed somewhere between then and 7.1/master?

@machty
Copy link
Contributor Author

machty commented Jan 8, 2022

Adding the following (somewhat obviously) circumvents the issue:

    def respond_to_missing?(name, include_private = false)
      return false if name == :before_remove_const

      super
    end

@machty machty changed the title Rails master: issues reloading a class that uses delegate_missing_to Rails master: issues reloading a class that uses delegate_missing_to and references other class Jan 8, 2022
@machty machty changed the title Rails master: issues reloading a class that uses delegate_missing_to and references other class Rails 7: issues reloading a class that uses delegate_missing_to and references other class Jan 17, 2022
@fxn
Copy link
Member

fxn commented Mar 20, 2022

I missed this one!

Your analysis is correct. You didn't find this in 6.1 because before_remove_const was not emulated in zeitwerk mode, it was restored for Rails 7.

This one is a bit tricky. Unloading is documented to happen in an undefined order. Documentation also says that, in consequence, on_unload cannot rely on the existence of other reloadable constants.

However, your code is so far away from all this!

  1. The on_unload hook could invoke before_remove_const and catch NoMethodError. Problem is, this is 21x times slower. We go from 19M i/s down to 874K i/s in my benchmarks.
  2. We could invoke respond_to? and catch any exception that might raise meaning: OK, we cannot determine if this object implements before_remove_const, so we pass. However, this could silence real errors the user should see.
  3. This is such a corner case, that perhaps we could address it in the documentation of delegate_missing_to.
  4. ...

Let me think a bit about it.

@casperisfine In my benchmarks, (1) is executed at about 874K i/s, do you think it would be noticeable at your scale?

@fxn
Copy link
Member

fxn commented Mar 20, 2022

Another option is to eliminate this feature. It was missing in Rails 6.0 and Rails 6.1, nobody complained. It is not documented anywhere, only in a CHANGELOG of 3.1. Rails only uses it to reset AR scopes.

Also, I don't like it. I don't like infra code mixed with business logic APIs, prefer callbacks (that is why Zeitwerk does not have this feature).

@fxn fxn closed this as completed in 7d87086 Mar 20, 2022
fxn added a commit that referenced this issue Mar 20, 2022
Rails 6.0 and Rails 6.1 didn't support the undocumented `before_remove_const` in
`zeitwerk` mode. I noticed this cleanup in AR was not being executed, and
restored the original code for Rails 7.

However, invoking `respond_to?` in an `on_unload` callback may have unexpected
side-effects, as seen in #44125. So, this patch reimplements the cleanup in a
more modern way.

Fixes #44125.
@fxn
Copy link
Member

fxn commented Mar 20, 2022

I have backported the fix to 7-0-stable.

@casperisfine please disregard the question :).

@fxn
Copy link
Member

fxn commented Mar 20, 2022

Just in case, to round it, I have also added a mention to before_remove_const in the Classic to Zeitwerk HOWTO guide.

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

No branches or pull requests

2 participants