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

Autoloading regression between Rails 7.0 and 7.1 #49629

Closed
bradgessler opened this issue Oct 13, 2023 · 5 comments
Closed

Autoloading regression between Rails 7.0 and 7.1 #49629

bradgessler opened this issue Oct 13, 2023 · 5 comments
Labels

Comments

@bradgessler
Copy link

Steps to reproduce

Setup the Rails 7.1 project:

git clone git@github.com:bradgessler/rails-7-1-autoloading-regression.git
cd rails-7-1-autoloading-regression.git
bundle
bin/rails server

Then open http://localhost:3000. You should see the error. Refresh the page and you won't see the error.

Now open ./config/environments/development.rb and change config.eager_load

  config.eager_load = true

Reboot the server and you'll see the Zeitwerk error message.

The path for this helper is set at https://github.com/sitepress/sitepress/blob/main/sitepress-rails/lib/sitepress/engine.rb#L29 and it lives in the Rails app at https://github.com/bradgessler/rails-7-1-autoloading-regression/blob/main/app/content/helpers/page_helper.rb

To see the behavior in Rails 7.0, run:

git checkout -b rails-7-1
bundle
bin/rails server

Load the page with the eager_load setting to true, then false, and you'll see that it works.

Expected behavior

I expect for the dev environment is one of the following:

  1. It continues to work, as it does in Rails 7.0
  2. The Zeitwerk autoloading error is displayed immediately in the dev env

Actual behavior

In my dev env, the helper is not initially loaded. When I refresh the page it's loaded.

Video of this behavior at https://objects.bradgessler.com/Screen-Recording-2023-10-13-at-12.09.03-PM.mov

System configuration

Rails version: 7.1.1

Ruby version: 3.2

@fxn
Copy link
Member

fxn commented Oct 13, 2023

Got it!

This is actually unrelated to autoloading or Zeitwerk (though you could not know :). If you comment config.autoload_lib out in config/application.rb, everything works.

The fundamental issue has been in Rails for a long time, let me explain:

  1. Whenever you access config.paths, an object is memoized.
  2. Whenever you access one particular path collection, like config.autoload_paths, the compiled autoload paths at the time config.paths was accessed is filtered, and the result in turn and memoized, here.
  3. Therefore, if you change a config.paths["..."] later, it won't have any effect.
  4. So, unfortunately, this paths support in Rails is implemented in a way that is not deterministic, because it depends on whether someone accessed the paths before (config.autoload_lib does), and that is out of your control.

Note that config.autoload_lib is not to blame, it is just doing what any user could do manually. The root cause is all those memoizations and the lack of guarantees your mutations will have any effect, which is broken.

We'll need to address this some way or another, because config.autoload_lib being generated by default is going to surface it even more.

@fxn
Copy link
Member

fxn commented Oct 14, 2023

So, a minimal reproduction of the root issue in a Rails 7.0 application is this:

  1. Generate a new 7.0 application.

  2. In config/application.rb add these two lines:

     config.paths["app/helpers"] << "#{Rails.root}/vendor"
     config.eager_load_paths << "#{Rails.root}/lib"
  3. Inspect bin/rails r 'pp ActiveSupport::Dependencies.autoload_paths'.

You'll see vendor included, as expected.

Now, order the lines the other way around:

config.eager_load_paths << "#{Rails.root}/lib"
config.paths["app/helpers"] << "#{Rails.root}/vendor"

and run the command again. You'll see vendor is no longer listed.

Of course, from the point of view of the user, the order of those lines should make no difference. That is broken.

I have not verified personally, but from reading the source code, I'd bet this has worked this way since Rails 3.0, where this implementation with memoizations was added.

@fxn
Copy link
Member

fxn commented Oct 14, 2023

Fix coming soon.

@fxn
Copy link
Member

fxn commented Oct 14, 2023

This is fixed Brad, thanks very much for such a great issue description and simple reproduction steps.

@fxn fxn closed this as completed Oct 14, 2023
@bradgessler
Copy link
Author

I always learn a thing or two about the internals of Rails when I open a PR for you—thanks for the clear description of the problems and the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants