Merge remote-tracking branch 'origin/master' into actioncable_logging

7 files changed, 62 insertions, 31 deletions

diff --git a/actionpack/lib/action_controller/log_subscriber.rb b/actionpack/lib/action_controller/log_subscriber.rb
index 4c9f14e409..a0917b4fdb 100644
--- a/actionpack/lib/action_controller/log_subscriber.rb
+++ b/actionpack/lib/action_controller/log_subscriber.rb
@@ -25,7 +25,9 @@ module ActionController
           status = ActionDispatch::ExceptionWrapper.status_code_for_exception(exception_class_name)
         end
         message = "Completed #{status} #{Rack::Utils::HTTP_STATUS_CODES[status]} in #{event.duration.round}ms"
-        message << " (#{additions.join(" | ".freeze)})" unless additions.blank?
+        message << " (#{additions.join(" | ".freeze)})" unless additions.empty?
+        message << "\n\n" if defined?(Rails.env) && Rails.env.development?
+
         message
       end
     end
diff --git a/actionpack/lib/action_controller/metal.rb b/actionpack/lib/action_controller/metal.rb
index f6a93a8940..1641d01c30 100644
--- a/actionpack/lib/action_controller/metal.rb
+++ b/actionpack/lib/action_controller/metal.rb
@@ -174,6 +174,7 @@ module ActionController
     def response_body=(body)
       body = [body] unless body.nil? || body.respond_to?(:each)
       response.reset_body!
+      return unless body
       body.each { |part|
         next if part.empty?
         response.write part
diff --git a/actionpack/lib/action_controller/metal/instrumentation.rb b/actionpack/lib/action_controller/metal/instrumentation.rb
index 3dbf34eb2a..bf74b39ac4 100644
--- a/actionpack/lib/action_controller/metal/instrumentation.rb
+++ b/actionpack/lib/action_controller/metal/instrumentation.rb
@@ -19,9 +19,9 @@ module ActionController
         :controller => self.class.name,
         :action     => self.action_name,
         :params     => request.filtered_parameters,
-        :format     => request.format.try(:ref),
+        :format     => request.format.ref,
         :method     => request.request_method,
-        :path       => (request.fullpath rescue "unknown")
+        :path       => request.fullpath
       }
 
       ActiveSupport::Notifications.instrument("start_processing.action_controller", raw_payload.dup)
diff --git a/actionpack/lib/action_controller/metal/live.rb b/actionpack/lib/action_controller/metal/live.rb
index e3c540bf5f..fc20e7a421 100644
--- a/actionpack/lib/action_controller/metal/live.rb
+++ b/actionpack/lib/action_controller/metal/live.rb
@@ -237,39 +237,55 @@ module ActionController
       # This processes the action in a child thread. It lets us return the
       # response code and headers back up the rack stack, and still process
       # the body in parallel with sending data to the client
-      Thread.new {
-        t2 = Thread.current
-        t2.abort_on_exception = true
-
-        # Since we're processing the view in a different thread, copy the
-        # thread locals from the main thread to the child thread. :'(
-        locals.each { |k,v| t2[k] = v }
-
-        begin
-          super(name)
-        rescue => e
-          if @_response.committed?
-            begin
-              @_response.stream.write(ActionView::Base.streaming_completion_on_exception) if request.format == :html
-              @_response.stream.call_on_error
-            rescue => exception
-              log_error(exception)
-            ensure
-              log_error(e)
-              @_response.stream.close
+      new_controller_thread {
+        ActiveSupport::Dependencies.interlock.running do
+          t2 = Thread.current
+
+          # Since we're processing the view in a different thread, copy the
+          # thread locals from the main thread to the child thread. :'(
+          locals.each { |k,v| t2[k] = v }
+
+          begin
+            super(name)
+          rescue => e
+            if @_response.committed?
+              begin
+                @_response.stream.write(ActionView::Base.streaming_completion_on_exception) if request.format == :html
+                @_response.stream.call_on_error
+              rescue => exception
+                log_error(exception)
+              ensure
+                log_error(e)
+                @_response.stream.close
+              end
+            else
+              error = e
             end
-          else
-            error = e
+          ensure
+            @_response.commit!
           end
-        ensure
-          @_response.commit!
         end
       }
 
-      @_response.await_commit
+      ActiveSupport::Dependencies.interlock.permit_concurrent_loads do
+        @_response.await_commit
+      end
+
       raise error if error
     end
 
+    # Spawn a new thread to serve up the controller in.  This is to get
+    # around the fact that Rack isn't based around IOs and we need to use
+    # a thread to stream data from the response bodies.  Nobody should call
+    # this method except in Rails internals. Seriously!
+    def new_controller_thread # :nodoc:
+      Thread.new {
+        t2 = Thread.current
+        t2.abort_on_exception = true
+        yield
+      }
+    end
+
     def log_error(exception)
       logger = ActionController::Base.logger
       return unless logger
diff --git a/actionpack/lib/action_controller/metal/request_forgery_protection.rb b/actionpack/lib/action_controller/metal/request_forgery_protection.rb
index 91b3403ad5..6586985ff5 100644
--- a/actionpack/lib/action_controller/metal/request_forgery_protection.rb
+++ b/actionpack/lib/action_controller/metal/request_forgery_protection.rb
@@ -378,7 +378,8 @@ module ActionController #:nodoc:
       end
 
       def xor_byte_strings(s1, s2)
-        s1.bytes.zip(s2.bytes).map { |(c1,c2)| c1 ^ c2 }.pack('c*')
+        s2_bytes = s2.bytes
+        s1.bytes.map.with_index { |c1, i| c1 ^ s2_bytes[i] }.pack('c*')
       end
 
       # The form's authenticity parameter. Override to provide your own.
diff --git a/actionpack/lib/action_controller/metal/strong_parameters.rb b/actionpack/lib/action_controller/metal/strong_parameters.rb
index ba03430930..d3382ef296 100644
--- a/actionpack/lib/action_controller/metal/strong_parameters.rb
+++ b/actionpack/lib/action_controller/metal/strong_parameters.rb
@@ -587,11 +587,11 @@ module ActionController
     def method_missing(method_sym, *args, &block)
       if @parameters.respond_to?(method_sym)
         message = <<-DEPRECATE.squish
-          Method #{ method_sym } is deprecated and will be removed in Rails 5.1,
+          Method #{method_sym} is deprecated and will be removed in Rails 5.1,
           as `ActionController::Parameters` no longer inherits from
           hash. Using this deprecated behavior exposes potential security
           problems. If you continue to use this method you may be creating
-          a security vulunerability in your app that can be exploited. Instead,
+          a security vulnerability in your app that can be exploited. Instead,
           consider using one of these documented methods which are not
           deprecated: http://api.rubyonrails.org/v#{ActionPack.version}/classes/ActionController/Parameters.html
         DEPRECATE
diff --git a/actionpack/lib/action_controller/test_case.rb b/actionpack/lib/action_controller/test_case.rb
index b43bb9dc17..0c4b661214 100644
--- a/actionpack/lib/action_controller/test_case.rb
+++ b/actionpack/lib/action_controller/test_case.rb
@@ -12,6 +12,17 @@ module ActionController
     include Testing::Functional
   end
 
+  module Live
+    # Disable controller / rendering threads in tests.  User tests can access
+    # the database on the main thread, so they could open a txn, then the
+    # controller thread will open a new connection and try to access data
+    # that's only visible to the main thread's txn.  This is the problem in #23483
+    remove_method :new_controller_thread
+    def new_controller_thread # :nodoc:
+      yield
+    end
+  end
+
   # ActionController::TestCase will be deprecated and moved to a gem in Rails 5.1.
   # Please use ActionDispatch::IntegrationTest going forward.
   class TestRequest < ActionDispatch::TestRequest #:nodoc: