Skip to content

Commit

Permalink
disable controller / view thread spawning in tests
Browse files Browse the repository at this point in the history
Tests can (and do) access the database from the main thread.  In this
case they were starting a transaction, then making a request.  The
request would create a new thread, which would allocate a new database
connection.  Since the main thread started a transaction that contains
data that the new thread wants to see, the new thread would not see it
due to data visibility from transactions.  Spawning the new thread in
production is fine because middleware should not be doing database
manipulation similar to the test harness.  Before 603fe20 it was
possible to set the database connection id based on a thread local, but
603fe20 changes the connection lookup code to never look at the
"connection id" but only at the thread object itself.  Without that
indirection, we can't force threads to use the same connection pool as
another thread.

Fixes rails#23483
  • Loading branch information
tenderlove committed Feb 5, 2016
1 parent f4f998d commit a640da4
Showing 3 changed files with 31 additions and 3 deletions.
15 changes: 13 additions & 2 deletions actionpack/lib/action_controller/metal/live.rb
Original file line number Diff line number Diff line change
@@ -237,9 +237,8 @@ def process(name)
# 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 {
new_controller_thread {
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. :'(
@@ -270,6 +269,18 @@ def process(name)
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
10 changes: 10 additions & 0 deletions actionpack/lib/action_controller/test_case.rb
Original file line number Diff line number Diff line change
@@ -12,6 +12,16 @@ class Metal
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
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:
9 changes: 8 additions & 1 deletion actionpack/test/controller/live_stream_test.rb
Original file line number Diff line number Diff line change
@@ -152,7 +152,6 @@ def blocking_stream

def thread_locals
tc.assert_equal 'aaron', Thread.current[:setting]
tc.assert_not_equal Thread.current.object_id, Thread.current[:originating_thread]

response.headers['Content-Type'] = 'text/event-stream'
%w{ hello world }.each do |word|
@@ -261,6 +260,14 @@ def capture_log_output
end
end

def setup
super

def @controller.new_controller_thread
Thread.new { yield }
end
end

def test_set_cookie
get :set_cookie
assert_equal({'hello' => 'world'}, @response.cookies)

0 comments on commit a640da4

Please sign in to comment.