Clear url helper methods when routes are reloaded

Clear url helper methods when routes are reloaded by removing the
methods explicitly rather than just clearing the module because it
didn't work properly and could be the source of a memory leak.

Closes #8488.

4 files changed, 175 insertions, 6 deletions

diff --git a/actionpack/CHANGELOG.md b/actionpack/CHANGELOG.md
index ea82263c1a..a681a2dc79 100644
--- a/actionpack/CHANGELOG.md
+++ b/actionpack/CHANGELOG.md
@@ -1,5 +1,11 @@
 ## Rails 3.2.10 (unreleased) ##
 
+*   Clear url helper methods when routes are reloaded by removing the methods
+    explicitly rather than just clearing the module because it didn't work
+    properly and could be the source of a memory leak.
+
+    *Andrew White*
+
 *   Fix a bug in ActionDispatch::Request#raw_post that caused env['rack.input']
     to be read but not rewound.
 
diff --git a/actionpack/lib/action_dispatch/routing/route_set.rb b/actionpack/lib/action_dispatch/routing/route_set.rb
index 8168bd4fcc..a993699e05 100644
--- a/actionpack/lib/action_dispatch/routing/route_set.rb
+++ b/actionpack/lib/action_dispatch/routing/route_set.rb
@@ -94,7 +94,12 @@ module ActionDispatch
         attr_reader :routes, :helpers, :module
 
         def initialize
-          clear!
+          @routes = {}
+          @helpers = []
+
+          @module = Module.new do
+            instance_methods.each { |selector| remove_method(selector) }
+          end
         end
 
         def helper_names
@@ -102,12 +107,14 @@ module ActionDispatch
         end
 
         def clear!
+          @helpers.each do |helper|
+            @module.module_eval do
+              remove_possible_method helper
+            end
+          end
+
           @routes = {}
           @helpers = []
-
-          @module ||= Module.new do
-            instance_methods.each { |selector| remove_method(selector) }
-          end
         end
 
         def add(name, route)
@@ -291,7 +298,6 @@ module ActionDispatch
 
       def clear!
         @finalized = false
-        @url_helpers = nil
         named_routes.clear
         set.clear
         formatter.clear
diff --git a/actionpack/test/dispatch/routing/route_set_test.rb b/actionpack/test/dispatch/routing/route_set_test.rb
new file mode 100644
index 0000000000..d57b1a5637
--- /dev/null
+++ b/actionpack/test/dispatch/routing/route_set_test.rb
@@ -0,0 +1,86 @@
+require 'abstract_unit'
+
+module ActionDispatch
+  module Routing
+    class RouteSetTest < ActiveSupport::TestCase
+      class SimpleApp
+        def initialize(response)
+          @response = response
+        end
+
+        def call(env)
+          [ 200, { 'Content-Type' => 'text/plain' }, [response] ]
+        end
+      end
+
+      setup do
+        @set = RouteSet.new
+      end
+
+      test "url helpers are added when route is added" do
+        draw do
+          get 'foo', to: SimpleApp.new('foo#index')
+        end
+
+        assert_equal '/foo', url_helpers.foo_path
+        assert_raises NoMethodError do
+          assert_equal '/bar', url_helpers.bar_path
+        end
+
+        draw do
+          get 'foo', to: SimpleApp.new('foo#index')
+          get 'bar', to: SimpleApp.new('bar#index')
+        end
+
+        assert_equal '/foo', url_helpers.foo_path
+        assert_equal '/bar', url_helpers.bar_path
+      end
+
+      test "url helpers are updated when route is updated" do
+        draw do
+          get 'bar', to: SimpleApp.new('bar#index'), as: :bar
+        end
+
+        assert_equal '/bar', url_helpers.bar_path
+
+        draw do
+          get 'baz', to: SimpleApp.new('baz#index'), as: :bar
+        end
+
+        assert_equal '/baz', url_helpers.bar_path
+      end
+
+      test "url helpers are removed when route is removed" do
+        draw do
+          get 'foo', to: SimpleApp.new('foo#index')
+          get 'bar', to: SimpleApp.new('bar#index')
+        end
+
+        assert_equal '/foo', url_helpers.foo_path
+        assert_equal '/bar', url_helpers.bar_path
+
+        draw do
+          get 'foo', to: SimpleApp.new('foo#index')
+        end
+
+        assert_equal '/foo', url_helpers.foo_path
+        assert_raises NoMethodError do
+          assert_equal '/bar', url_helpers.bar_path
+        end
+      end
+
+      private
+        def clear!
+          @set.clear!
+        end
+
+        def draw(&block)
+          @set.draw(&block)
+        end
+
+        def url_helpers
+          @set.url_helpers
+        end
+    end
+  end
+end
diff --git a/railties/test/application/routing_test.rb b/railties/test/application/routing_test.rb
index a05e39658d..07807c3620 100644
--- a/railties/test/application/routing_test.rb
+++ b/railties/test/application/routing_test.rb
@@ -229,6 +229,77 @@ module ApplicationTests
       end
     end
 
+    test 'routes are added and removed when reloading' do
+      app('development')
+
+      controller :foo, <<-RUBY
+        class FooController < ApplicationController
+          def index
+            render text: "foo"
+          end
+        end
+      RUBY
+
+      controller :bar, <<-RUBY
+        class BarController < ApplicationController
+          def index
+            render text: "bar"
+          end
+        end
+      RUBY
+
+      app_file 'config/routes.rb', <<-RUBY
+        AppTemplate::Application.routes.draw do
+          get 'foo', to: 'foo#index'
+        end
+      RUBY
+
+      get '/foo'
+      assert_equal 'foo', last_response.body
+      assert_equal '/foo', Rails.application.routes.url_helpers.foo_path
+
+      get '/bar'
+      assert_equal 404, last_response.status
+      assert_raises NoMethodError do
+        assert_equal '/bar', Rails.application.routes.url_helpers.bar_path
+      end
+
+      app_file 'config/routes.rb', <<-RUBY
+        AppTemplate::Application.routes.draw do
+          get 'foo', to: 'foo#index'
+          get 'bar', to: 'bar#index'
+        end
+      RUBY
+
+      Rails.application.reload_routes!
+
+      get '/foo'
+      assert_equal 'foo', last_response.body
+      assert_equal '/foo', Rails.application.routes.url_helpers.foo_path
+
+      get '/bar'
+      assert_equal 'bar', last_response.body
+      assert_equal '/bar', Rails.application.routes.url_helpers.bar_path
+
+      app_file 'config/routes.rb', <<-RUBY
+        AppTemplate::Application.routes.draw do
+          get 'foo', to: 'foo#index'
+        end
+      RUBY
+
+      Rails.application.reload_routes!
+
+      get '/foo'
+      assert_equal 'foo', last_response.body
+      assert_equal '/foo', Rails.application.routes.url_helpers.foo_path
+
+      get '/bar'
+      assert_equal 404, last_response.status
+      assert_raises NoMethodError do
+        assert_equal '/bar', Rails.application.routes.url_helpers.bar_path
+      end
+    end
+
     test 'resource routing with irregular inflection' do
       app_file 'config/initializers/inflection.rb', <<-RUBY
         ActiveSupport::Inflector.inflections do |inflect|