Eagerly build the routing helper module after routes are committed

This commit eagerly builds the route helper module after the routes have
been drawn and finalized.  This allows us to cache the helper module but
not have to worry about people accessing the module while route
definition is "in-flight", and automatically deals with cache
invalidation as the module is regenerated anytime someone redraws the
routes.

The restriction this commit introduces is that the url helper module can
only be accessed *after* the routes are done being drawn.

Refs #24554 and #32892

7 files changed, 62 insertions, 27 deletions

diff --git a/actionpack/lib/action_dispatch/routing/mapper.rb b/actionpack/lib/action_dispatch/routing/mapper.rb
index 3f7cf0950d..b618b9c400 100644
--- a/actionpack/lib/action_dispatch/routing/mapper.rb
+++ b/actionpack/lib/action_dispatch/routing/mapper.rb
@@ -664,7 +664,6 @@ module ActionDispatch
           def define_generate_prefix(app, name)
             _route = @set.named_routes.get name
             _routes = @set
-            _url_helpers = @set.url_helpers
 
             script_namer = ->(options) do
               prefix_options = options.slice(*_route.segment_keys)
@@ -676,7 +675,7 @@ module ActionDispatch
 
               # We must actually delete prefix segment keys to avoid passing them to next url_for.
               _route.segment_keys.each { |k| options.delete(k) }
-              _url_helpers.send("#{name}_path", prefix_options)
+              @set.url_helpers.send("#{name}_path", prefix_options)
             end
 
             app.routes.define_mounted_helper(name, script_namer)
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index acce8a7ef3..8f21722a6a 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -378,6 +378,7 @@ module ActionDispatch
         @disable_clear_and_finalize = false
         @finalized                  = false
         @env_key                    = "ROUTES_#{object_id}_SCRIPT_NAME".freeze
+        @url_helpers                = nil
 
         @set    = Journey::Routes.new
         @router = Journey::Router.new @set
@@ -437,6 +438,7 @@ module ActionDispatch
         return if @finalized
         @append.each { |blk| eval_block(blk) }
         @finalized = true
+        @url_helpers = build_url_helper_module true
       end
 
       def clear!
@@ -465,11 +467,10 @@ module ActionDispatch
         return if MountedHelpers.method_defined?(name)
 
         routes = self
-        helpers = routes.url_helpers
 
         MountedHelpers.class_eval do
           define_method "_#{name}" do
-            RoutesProxy.new(routes, _routes_context, helpers, script_namer)
+            RoutesProxy.new(routes, _routes_context, routes.url_helpers, script_namer)
           end
         end
 
@@ -480,7 +481,20 @@ module ActionDispatch
         RUBY
       end
 
+      class UnfinalizedRouteSet < StandardError
+      end
+
       def url_helpers(supports_path = true)
+        raise UnfinalizedRouteSet, "routes have not been finalized. Please call `finalize!` or use `draw(&block)`" unless @finalized
+
+        if supports_path
+          @url_helpers
+        else
+          build_url_helper_module false
+        end
+      end
+
+      def build_url_helper_module(supports_path)
         routes = self
 
         Module.new do
diff --git a/actionpack/lib/action_dispatch/testing/assertions/routing.rb b/actionpack/lib/action_dispatch/testing/assertions/routing.rb
index af41521c5c..0e8712f8d9 100644
--- a/actionpack/lib/action_dispatch/testing/assertions/routing.rb
+++ b/actionpack/lib/action_dispatch/testing/assertions/routing.rb
@@ -138,6 +138,20 @@ module ActionDispatch
         assert_generates(path.is_a?(Hash) ? path[:path] : path, generate_options, defaults, extras, message)
       end
 
+      # Provides a hook on `finalize!` so we can mutate a controller after the
+      # route set has been drawn.
+      class WithRouting < ActionDispatch::Routing::RouteSet # :nodoc:
+        def initialize(&block)
+          super()
+          @block = block
+        end
+
+        def finalize!
+          super
+          @block.call self
+        end
+      end
+
       # A helper to make it easier to test different route configurations.
       # This method temporarily replaces @routes with a new RouteSet instance.
       #
@@ -152,16 +166,19 @@ module ActionDispatch
       #   end
       #
       def with_routing
-        old_routes, @routes = @routes, ActionDispatch::Routing::RouteSet.new
-        if defined?(@controller) && @controller
-          old_controller, @controller = @controller, @controller.clone
-          _routes = @routes
-
-          @controller.singleton_class.include(_routes.url_helpers)
-
-          if @controller.respond_to? :view_context_class
-            @controller.view_context_class = Class.new(@controller.view_context_class) do
-              include _routes.url_helpers
+        old_routes = @routes
+        old_controller = nil
+        @routes = WithRouting.new do |_routes|
+          if defined?(@controller) && @controller
+            old_controller, @controller = @controller, @controller.clone
+            _routes = @routes
+
+            @controller.singleton_class.include(_routes.url_helpers)
+
+            if @controller.respond_to? :view_context_class
+              @controller.view_context_class = Class.new(@controller.view_context_class) do
+                include _routes.url_helpers
+              end
             end
           end
         end
diff --git a/actionpack/test/abstract_unit.rb b/actionpack/test/abstract_unit.rb
index 65dd28b3d7..7c9f15108d 100644
--- a/actionpack/test/abstract_unit.rb
+++ b/actionpack/test/abstract_unit.rb
@@ -100,7 +100,10 @@ end
 
 class ActionDispatch::IntegrationTest < ActiveSupport::TestCase
   def self.build_app(routes = nil)
-    RoutedRackApp.new(routes || ActionDispatch::Routing::RouteSet.new) do |middleware|
+    routes ||= ActionDispatch::Routing::RouteSet.new.tap { |rs|
+      rs.draw { }
+    }
+    RoutedRackApp.new(routes) do |middleware|
       middleware.use ActionDispatch::ShowExceptions, ActionDispatch::PublicExceptions.new("#{FIXTURE_LOAD_PATH}/public")
       middleware.use ActionDispatch::DebugExceptions
       middleware.use ActionDispatch::Callbacks
diff --git a/actionpack/test/controller/integration_test.rb b/actionpack/test/controller/integration_test.rb
index 39ede1442a..b078e8ad9f 100644
--- a/actionpack/test/controller/integration_test.rb
+++ b/actionpack/test/controller/integration_test.rb
@@ -542,9 +542,6 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest
     def with_test_route_set
       with_routing do |set|
         controller = ::IntegrationProcessTest::IntegrationController.clone
-        controller.class_eval do
-          include set.url_helpers
-        end
 
         set.draw do
           get "moved" => redirect("/method")
@@ -555,6 +552,10 @@ class IntegrationProcessTest < ActionDispatch::IntegrationTest
           end
         end
 
+        controller.class_eval do
+          include set.url_helpers
+        end
+
         singleton_class.include(set.url_helpers)
 
         yield
diff --git a/actionpack/test/dispatch/routing/ipv6_redirect_test.rb b/actionpack/test/dispatch/routing/ipv6_redirect_test.rb
index 31559bffc7..fb423d2951 100644
--- a/actionpack/test/dispatch/routing/ipv6_redirect_test.rb
+++ b/actionpack/test/dispatch/routing/ipv6_redirect_test.rb
@@ -4,6 +4,11 @@ require "abstract_unit"
 
 class IPv6IntegrationTest < ActionDispatch::IntegrationTest
   Routes = ActionDispatch::Routing::RouteSet.new
+  Routes.draw do
+    get "/",    to: "bad_route_request#index", as: :index
+    get "/foo", to: "bad_route_request#foo", as: :foo
+  end
+
   include Routes.url_helpers
 
   class ::BadRouteRequestController < ActionController::Base
@@ -17,11 +22,6 @@ class IPv6IntegrationTest < ActionDispatch::IntegrationTest
     end
   end
 
-  Routes.draw do
-    get "/",    to: "bad_route_request#index", as: :index
-    get "/foo", to: "bad_route_request#foo", as: :foo
-  end
-
   def _routes
     Routes
   end
diff --git a/actionpack/test/dispatch/url_generation_test.rb b/actionpack/test/dispatch/url_generation_test.rb
index aef9351de1..b0096d26be 100644
--- a/actionpack/test/dispatch/url_generation_test.rb
+++ b/actionpack/test/dispatch/url_generation_test.rb
@@ -4,16 +4,13 @@ require "abstract_unit"
 
 module TestUrlGeneration
   class WithMountPoint < ActionDispatch::IntegrationTest
-    Routes = ActionDispatch::Routing::RouteSet.new
-    include Routes.url_helpers
-
     class ::MyRouteGeneratingController < ActionController::Base
-      include Routes.url_helpers
       def index
         render plain: foo_path
       end
     end
 
+    Routes = ActionDispatch::Routing::RouteSet.new
     Routes.draw do
       get "/foo", to: "my_route_generating#index", as: :foo
 
@@ -22,6 +19,10 @@ module TestUrlGeneration
       mount MyRouteGeneratingController.action(:index), at: "/bar"
     end
 
+    class ::MyRouteGeneratingController
+      include Routes.url_helpers
+    end
+
     APP = build_app Routes
 
     def _routes