Fully initialize routes before the first request is handled #27647

casperisfine · 2017-01-11T14:06:48Z

AD::Journey::GTG::Simulator is lazily built the first time Journey::Router#find_routes is invoked, which happens when the first request is served.

On small applications with only a few dozen routes, it only takes a few milliseconds. But on large applications with hundreds of routes, building the simulator can take several hundred milliseconds (~700ms for us).

Triggering this initialization during the boot process reduces the impact of deploys on the application response time.

@rafaelfranca

rails-bot · 2017-01-11T14:06:59Z

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @arthurnn (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

casperisfine · 2017-01-11T14:18:56Z

Hum, triggering the initialization after set_routes_reloader_hook worked on 5.0. Not sure what changed in 5.1 that makes it break now. I'll try to figure that out.

casperisfine · 2017-01-11T15:36:28Z

It's fixed now.

rafaelfranca · 2017-01-11T15:49:47Z

railties/lib/rails/application/finisher.rb

@@ -138,6 +138,9 @@ def self.complete(_state)
          # some sort of reloaders dependency support, to be added.
          require_unload_lock!
          reloader.execute
+          if app.config.eager_load
+            app.routes.eager_load!


What is the reason for being in this initializer?

It can be in another one. But it has to be called after reloader.execute so that routes are loaded.

I figured since this is just a byproduct of loading routes, it would fit nicely there.

I see. Yeah, it make sense.

Maybe we should move the eager_load! call to the reloader like this:

def eager_load! execute route_sets.each(&:eager_load!) end

And here

if app.config.eager_load reloader.eager_load! else reloader.execute end

I'd think about moving the eager_load config into the routes reloader, and having execute do the eager load, instead: it's not a different thing a caller might ask it to do, it's a different set of behaviour for it to perform, based on configuration, when told to reload.

@matthewd I tried to do that in 9cf3897. Let me know what you think.

rafaelfranca · 2017-01-11T15:50:21Z

cc @pixeltrix I think you commented something about this before.

rafaelfranca · 2017-01-11T15:55:49Z

railties/lib/rails/application/finisher.rb

@@ -138,6 +138,9 @@ def self.complete(_state)
          # some sort of reloaders dependency support, to be added.
          require_unload_lock!
          reloader.execute
+          if app.config.eager_load
+            app.routes.eager_load!


I see. Yeah, it make sense.

pixeltrix · 2017-01-11T16:08:53Z

@rafaelfranca there's some instance variables in ActionDispatch::Journey::Path::Pattern that are lazily evaluated and can result in a NoMethodError on nil. Fixing those should be part of this PR as well - original issue is #23699.

casperisfine · 2017-01-12T13:34:22Z

@pixeltrix I tried to do it in 076a9124a7348a62decd013268fe24e3eb1f2ca6, let me know what you think.

matthewd · 2017-01-12T13:42:30Z

railties/lib/rails/application/routes_reloader.rb

@@ -19,6 +21,11 @@ def reload!
        revert
      end

+      def execute
+        updater.execute
+        route_sets.each(&:eager_load!)


Missing an if?

Absolutely 🤦‍♂️

pixeltrix · 2017-01-12T14:02:15Z

it looks like it'll work but pretty it ain't 😄

casperisfine · 2017-01-12T14:07:32Z

pretty it ain't

Yeah I agree :/

rafaelfranca · 2017-01-13T21:19:21Z

railties/lib/rails/application/routes_reloader.rb

@@ -4,11 +4,13 @@ module Rails
  class Application
    class RoutesReloader
      attr_reader :route_sets, :paths
-      delegate :execute_if_updated, :execute, :updated?, to: :updater
+      attr_accessor :eager_load
+      delegate :execute_if_updated, :updated?, to: :updater


I think execute_if_updated should also be defined here to run the eager load code.

@rafaelfranca good point. I fixed it.

rafaelfranca

Could you squash the commits?

`AD::Journey::GTG::Simulator` is lazily built the first time `Journey::Router#find_routes` is invoked, which happens when the first request is served. On large applications with many routes, building the simulator can take several hundred milliseconds (~700ms for us). Triggering this initialization during the boot process reduces the impact of deploys on the application response time.

casperisfine · 2017-01-18T10:11:32Z

@rafaelfranca squashed!

arthurnn · 2017-01-31T01:21:55Z

railties/lib/rails/application/finisher.rb

@@ -124,6 +124,7 @@ def self.complete(_state)
      # the hook are taken into account.
      initializer :set_routes_reloader_hook do |app|
        reloader = routes_reloader
+        reloader.eager_load = app.config.eager_load


couldn't we use railties eager_load method, instead of passing the config to action pack?
In 3.2, railties could define eager_load! method https://github.com/rails/rails/blob/3-2-stable/railties/lib/rails/engine.rb#L439 and it would get called. Not sure if that was removed or still exists.

Not sure what would it improve. Railtie also have eager_load_namespaces that automatically call eager_load! on them. But since the routes_reloader is created in an initializer we can't use it.

jkhart · 2017-02-10T23:23:10Z

Will this be included in a Rails 4.2.xx version? If so, which one?

rafaelfranca · 2017-02-10T23:47:11Z

No. It will be only included in 5.1

samarism · 2017-09-27T04:21:53Z

Hi, we are using jruby 1.7.25 with Rails 3.2.11. Rarely, on our prod servers, it seems that some of the Journey::Routes get corrupted. These corrupted routes/endpoints then always return an action_name as an empty string or nil. The only remedy is to restart the server.

This PR is fixing the race condition issue for a fresh start of the server by eager loading variables during the bootup process. Suppose however (under certain conditions) we need to trigger a Rails.application.reload_routes! in a running application. Won't the same race conditions reappear in that case because multiple threads might be processing the reload_routes! command.

jboler · 2018-05-31T05:15:29Z

No matter what I do, I can't get these new eager_load! methods to run. I have config.eager_load = true set in my current environment.

I've added debugging statements to initializer :set_routes_reloader_hook in finisher.rb, and all the other eager_load! methods.

reloader.execute_if_updated is executed at app start but does not trigger route eager loading because updater.execute_if_updated is false in RoutesReloader.

If I force a reload in the rails console with reload! then I get:

ArgumentError: unknown firstpos: NilClass
from /home/tenpin/.rvm/gems/jruby-9.1.17.0/gems/actionpack- 
5.1.5/lib/action_dispatch/journey/gtg/builder.rb:99:in `firstpos'

Am I missing something or is config.eager_load = true supposed to cause routes to be eager loaded at application start time?

jboler · 2018-05-31T15:53:42Z

I think #23699 & #33026 would be fixed if the routes were eager loaded.

rafaelfranca · 2018-05-31T20:02:13Z

Am I missing something or is config.eager_load = true supposed to cause routes to be eager loaded at application start time?

Yes, it is supposed to eager load it. If it is not it is a bug. Since you are investigating could you submit a PR?

prcongithub · 2019-10-11T01:10:03Z

Is this fix available on 5.2.0? I am getting this same error on JRuby-9.2.6.0, Rails-5.2.0, Puma 3.11.2

jboler · 2019-10-11T02:37:57Z

It's definitely fixed in 5.2.3 so I recommend upgrading to that. Upgrade puma & jRuby too :)

prcongithub · 2019-10-12T05:06:13Z

Tried that on our app and so far we have not see an error! Thanks!

Why this change? When running system tests on our CI, we have been occasionally seeing server errors like: ``` Error encountered while proccessing /stylesheets/desktop_e58cf7f686aab173f9b778797f241913c2833c39.css NoMethodError: undefined method `+' for nil:NilClass /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/path/pattern.rb:139:in `[]' /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:127:in `block (2 levels) in find_routes' /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `each' /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `each_with_index' /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:126:in `block in find_routes' /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:123:in `map!' /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:123:in `find_routes' /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/journey/router.rb:32:in `serve' /__w/discourse/discourse/vendor/bundle/ruby/3.2.0/gems/actionpack-7.0.7/lib/action_dispatch/routing/route_set.rb:852:in `call' ``` While looking through various Rails issues related to the error above, I came across rails/rails#27647 which is a fix to fully initialize routes before the first request is handled. However, the routes are only fully initialize only if `config.eager_load` is set to `true`. There is no reason why `config.eager_load` shouldn't be `true` in the CI environment and this is what a new Rails 7.1 app is generated with. What does this change do? Enable `config.eager_load` when `env["CI"]` is present

rails-bot assigned arthurnn Jan 11, 2017

prathamesh-sonpatki added actionpack railties labels Jan 11, 2017

casperisfine force-pushed the fully-eagerload-journey branch 2 times, most recently from 605954a to 9bac4dd Compare January 11, 2017 14:54

rafaelfranca reviewed Jan 11, 2017

View reviewed changes

rafaelfranca assigned rafaelfranca and unassigned arthurnn Jan 11, 2017

rafaelfranca approved these changes Jan 11, 2017

View reviewed changes

matthewd reviewed Jan 12, 2017

View reviewed changes

rafaelfranca reviewed Jan 13, 2017

View reviewed changes

casperisfine force-pushed the fully-eagerload-journey branch from c3aaa1c to 1924e72 Compare January 16, 2017 13:33

rafaelfranca approved these changes Jan 18, 2017

View reviewed changes

casperisfine force-pushed the fully-eagerload-journey branch from 1924e72 to 5b1332b Compare January 18, 2017 10:11

arthurnn reviewed Jan 31, 2017

View reviewed changes

rafaelfranca merged commit ce97c79 into rails:master Jan 31, 2017

rafaelfranca deleted the fully-eagerload-journey branch January 31, 2017 02:51

tomilaine mentioned this pull request Apr 6, 2018

ActionDispatch::Journey::Router NoMethodError (undefined method `names' for nil:NilClass) #23699

Closed

jboler mentioned this pull request Jun 4, 2018

Fix route eager loading #33054

Merged

tgxworld mentioned this pull request Dec 26, 2023

DEV: Set config.eager_load = true on CI discourse/discourse#25032

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully initialize routes before the first request is handled #27647

Fully initialize routes before the first request is handled #27647

casperisfine commented Jan 11, 2017

rails-bot commented Jan 11, 2017

casperisfine commented Jan 11, 2017

casperisfine commented Jan 11, 2017

rafaelfranca Jan 11, 2017

casperisfine Jan 11, 2017

rafaelfranca Jan 11, 2017

rafaelfranca Jan 11, 2017 •

edited

matthewd Jan 11, 2017

casperisfine Jan 12, 2017

rafaelfranca commented Jan 11, 2017

rafaelfranca Jan 11, 2017

pixeltrix commented Jan 11, 2017

casperisfine commented Jan 12, 2017

matthewd Jan 12, 2017

casperisfine Jan 12, 2017

casperisfine Jan 12, 2017

pixeltrix commented Jan 12, 2017

casperisfine commented Jan 12, 2017

rafaelfranca Jan 13, 2017

casperisfine Jan 16, 2017

rafaelfranca left a comment

casperisfine commented Jan 18, 2017

arthurnn Jan 31, 2017

rafaelfranca Jan 31, 2017

jkhart commented Feb 10, 2017

rafaelfranca commented Feb 10, 2017

samarism commented Sep 27, 2017

jboler commented May 31, 2018

jboler commented May 31, 2018

rafaelfranca commented May 31, 2018

prcongithub commented Oct 11, 2019 •

edited

jboler commented Oct 11, 2019

prcongithub commented Oct 12, 2019

Fully initialize routes before the first request is handled #27647

Fully initialize routes before the first request is handled #27647

Conversation

casperisfine commented Jan 11, 2017

rails-bot commented Jan 11, 2017

casperisfine commented Jan 11, 2017

casperisfine commented Jan 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelfranca Jan 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelfranca commented Jan 11, 2017

Choose a reason for hiding this comment

pixeltrix commented Jan 11, 2017

casperisfine commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pixeltrix commented Jan 12, 2017

casperisfine commented Jan 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rafaelfranca left a comment

Choose a reason for hiding this comment

casperisfine commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkhart commented Feb 10, 2017

rafaelfranca commented Feb 10, 2017

samarism commented Sep 27, 2017

jboler commented May 31, 2018

jboler commented May 31, 2018

rafaelfranca commented May 31, 2018

prcongithub commented Oct 11, 2019 • edited

jboler commented Oct 11, 2019

prcongithub commented Oct 12, 2019

rafaelfranca Jan 11, 2017 •

edited

prcongithub commented Oct 11, 2019 •

edited