Warn if frameworks are loaded too early

[p8]

Fixes #50133

Prematurely loading frameworks in a Rails application may slow down boot time and could cause conflicts with load order and boot of the application. It is a common problem in gems:

• 2.66 forces ActionView to load too soon ViewComponent/view_component#1507
• Delayed::Job no longer defined in initializers collectiveidea/delayed_job_active_record#185

This change allows showing a warning in a load hook if a required
framework or load hook hasn't been loaded yet:

  initializer "active_record.premature_loading_check" do
    ActiveSupport.on_load(:active_record) do
      ActiveSupport.warn_if_prematurely_loaded(:active_record, before: :after_initialize)
    end
  end

In this case if :active_record is loaded before :after_initialize
has run we show a warning:

  Load hook :active_record was called before load hook :after_initialize.
  Prematurely loading frameworks may slow down your boot time and could
  cause conflicts with load order and boot of your application.

  Consider wrapping your code with an on_load hook:

      ActiveSupport.on_load(:active_record) do
        # your code
      end
  Called from:
  config/initializers/some_initializer.rb:1:in `<main>'
  ...

In production eager_load gets called to load add frameworks and we
should silence the warning. A silence_prematurely_loading_warnings has
been added to silence all load hook order warnings. This
silence_prematurely_loading_warnings is called before eager_load
gets called.

The hook has been implemented for ActiveRecord but other frameworks,
like ActionView for example, could use it as well.

Some tests include initializers that call ActiveRecord::Base. To not
show the warning silence_prematurely_loading_warnings has been added
to the initializers.

Additional information

ActiveRecord should be loaded after :after_initialize has run, so I've added an initializer to add the load_hook order in the railtie. For other frameworks like ActionView and ActionPack it should probably be added as well.

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
• There are no typos in commit messages and comments.
• Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
• Feature branch is up-to-date with main (if not - rebase it).
• Pull request only contains one commit for bug fixes and small features. If it's a larger feature, multiple commits are permitted but must be descriptive.
• Tests are added if you fix a bug or add a feature.
• CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
• PR is not in a draft state.
• CI is passing.

[rafaelfranca]

Does this work in production? Because after_initialize runs after eager load is run, and eager loading would load all those frameworks, so we would see the warning. Would not we?

[p8]

Thanks for the review @rafaelfranca. I'll try to fix the issues you mentioned.

[p8]

Does this work in production? Because after_initialize runs after eager load is run, and eager loading would load all those frameworks, so we would see the warning. Would not we?

I've add ActiveSupport.silence_prematurely_loading_warnings to ignore all load hook order warnings.
This gets called in a before_configuration hook if eager_load is enabled.

[p8]

Ok, with eager_load the warning is still shown. I'll need to fix that.
Edit: a nevermind. It's working as expected.

[casperisfine]

Makes sense to be.

@rafaelfranca you good with this?

[casperisfine]

To clarify, I don't think that's ideal, I'd prefer if we addressed the root cause more rather than to warn that something is wrong, but I can't think of anything better yet and at least it should help people fix their app.

[fxn]

This feature seems odd to me.

If users need Active Record before the application has fully booted, how are they supposed to run the code and get no warnings?

[fxn]

For example, let's consider this widely used pattern:

config.to_prepare do
  # Setup an AR model on boot and reload here.
end

Which would be the alternative?

[p8]

@fxn I think the following works for me (but I don't have much experience in using to_prepare):

ActiveSupport::Reloader.to_prepare do
   ActiveSupport.on_load(:active_record) do
      # Setup an AR model on boot and reload here.
   end
end

[fxn]

@p8 But, if you execute bin/rails r 1, you'll see the block gets executed.

My point is, if I need to do something during boot, I need to. I cannot not do it because not doing it would be faster.

[p8]

@fxn We could allow the silencing to accept a block:

ActiveSupport::Reloader.to_prepare do
   ActiveSupport.silence_prematurely_loading_warnings do
      # Setup an AR model on boot and reload here.
   end
end

[fxn]

Smells like a XY solution to me, guess @casperisfine has a similar feeling from his comment.

Legit use cases cannot get warnings. Is like saying, don't use a database, it will slow you down! OK, but I need a database! Cool, here's a warning silencer!

[p8]

What I'm reading from @casperisfine's comment is that permaturely loading ActiveRecord shouldn't cause a problem.
I agree, but solving that seems like a big undertaking.
There is a whole section in the guides dedicated to explaining why we should use load hooks to avoid premature loading.
I've seen multiple cases (2 listed above) where prematurely loading a framework causes hard to debug errors.
This PR would at least help clarify the problem.

[fxn]

Note that while the server command does not load Active Record when the application boots, at least runner, console, and test do. So even if you configure all that, you save nothing in those cases.

I buy the "contract" argument, but not so much the performance argument.

[casperisfine]

that permaturely loading ActiveRecord shouldn't cause a problem.

Yes, either that, or that it would be build in such a way that people could hardly make that mistake. But neither seem achievable to me, at least not easily.

[fxn]

To me, on_load is: Rails controls when things are ready, you need to play by the rules and use this.

[p8]

There was an similar PR to solve this: #38024
This comment mentions previous reports of the same problem:

• Rails 5.0.0beta2: Weird behaviour of config belongs_to_required_by_default #23589
• Rails 5 belongs_to_required_by_default not working #27844
• new_framework_defaults loads too late #33283
• ActiveRecord::Base.cache_versioning is false even tough Rails.application.config.active_record.cache_versioning set to true #36322
• return_false_on_aborted_enqueue setting not working #37030

[fxn]

@p8 Yes! But they are not about performance, they are about load order, right? Some people comment on passing about booting, but that is just a potential consequence of delaying loading (potential only, as I showed in the Rails commands above).

What I believe would make sense is to be able to say: "Hey, you are trying to load AR before Rails!" (that is, a missing on_load). But that is not quite the same as avoid running during boot, and definitely don't see the warning about boot time. See what I mean?

I don't think the code is ready to be able to say that easily.

[fxn]

In other words, Rails is the one who decides when frameworks are loaded. And Rails may decide some have to be loaded at boot time (as it does for AR itself in some commands). Whether things are loaded during boot time or not is out of the control of the user regardless of whether they use on_load.

What the user has to respect is load order, wait until Rails decides to load.

[fxn]

And if you think about it, that is aligned with the eager loading case. The same warning works, you don't need special logic, because the point is to wait until frameworks are loaded, sooner or later.

[fxn]

I'll go even further. Let's imagine all the Rails code in the world uses on_load and 100% of users depend on it.

When should Rails load its components? Now, that question is extremely important because nobody will be able to run anything related before.

I'd argue that probably during boot, at some point in time that would belong to the public interface.

In the case of AR there are use cases like aborting an application from booting if the roles table lacks an admin. Say.

So, in order to support those use cases, AR should be available (ergo loaded) during initialization. Thus, defeating the performance goal.

The way I see it, in dev mode Rails has to be lean, but there's a low boundary to that. Being lean means being lazy loading the rest of AR, as it is done with all the autoloads.

And users have to play by the rules, which today are not enforced, but would be good to be able to.

[rafaelfranca]

And users have to play by the rules, which today are not enforced, but would be good to be able to.

Isn't what this warning is trying to do? If users code, or libraries are trying to load Rails owned frameworks before Rails thinks it should loaded, users will see a warning.

[fxn]

Isn't what this warning is trying to do? If users code, or libraries are trying to load Rails owned frameworks before Rails thinks it should loaded, users will see a warning.

Not really.

This patch is saying: If Active Record was loaded before :after_initialize, issue a warning. This is different. Rails considers AR to be loaded when the AS hook is triggered, which is a condition unrelated to :after_initialize. We have to recall that :after_initialize runs when the whole application and engines are initialized.

To accomplish what we are saying we should have something like: Now, I (Rails) am going to load framework F. If at this point F is already loaded, I am going to warn, and the warning won't mention performance. This logic does not depend on F.

[fxn]

Let me draft one possible way to do this.

1. In my view, it does not make sense that an application is booted and Rails is not ready to be used. The way I see it, conceptually, when initialization has finished, Rails, the application, and the engines, are ready.
2. Rails has to decide when it is going to load the frameworks. "Loading the frameworks" might sound heavy, but we are talking here mostly about evaluating some base.rb file with some require and autoload calls.
3. This has to belong to the public interface. For example, users need to know that if they want to perform a pre-flight check, that is going to happen at pre-flight time, not at some indeterminate time you don't control and could be already hitting controllers on paper. There has to be a contract.
4. I believe that point should happen at least before to_prepare runs, since autoloading models during to_prepare has been a blessed idiom for a long time.

Now, who has the responsibility to glue the frameworks? In Rails, the orchestrator is railties. In that sense, railties could install some initializer that has this logic:

for every framework F
  if the application is using F
    if F is not loaded
      load F
    else
      warn that F was loaded, bonus if you are able to say by who
    end
  end
end

Ideally, things should be done in a way that the warning is not needed because you simply cannot use the frameworks ealier by design. However, at this point in the game, that seems difficult. Also, there is a balance, because AR and others are independent gems, so other gems extending them do not even have a notion of on_load.

So, sharing this only for the sake of the discussion, it is not exactly a proposal because I am not sure about its practicality.

[p8]

Thanks @fxn for the extensive feedback. I hope to dive into this again later this week.

[runephilosof-abtion]

It seems to be stalled, so I have created an issue to track this #50133