Skip to content

Commit

Permalink
actionpack: Use an infinite sized queue in testing ActionController::…
Browse files Browse the repository at this point in the history
…Live

To avoid a deadlock from the buffer filling up during testing due to the
ActionController::Live test monkey patch that processes the request in
the same thread, which means there is no consumer of the queue until
the request is processed.
  • Loading branch information
dylanahsmith committed Jun 3, 2021
1 parent 3c165ad commit f6fb700
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 1 deletion.
4 changes: 4 additions & 0 deletions actionpack/CHANGELOG.md
@@ -1,3 +1,7 @@
* Fix ActionController::Live controller test deadlocks by removing the body buffer size limit for tests.

*Dylan Thacker-Smith*

* Drop support for the `SERVER_ADDR` header

Following up https://github.com/rack/rack/pull/1573 and https://github.com/rails/rails/pull/42349
Expand Down
11 changes: 10 additions & 1 deletion actionpack/lib/action_controller/metal/live.rb
Expand Up @@ -127,6 +127,11 @@ class ClientDisconnected < RuntimeError
class Buffer < ActionDispatch::Response::Buffer #:nodoc:
include MonitorMixin

class << self
attr_accessor :queue_size
end
@queue_size = 10

# Ignore that the client has disconnected.
#
# If this value is `true`, calling `write` after the client
Expand All @@ -136,7 +141,7 @@ class Buffer < ActionDispatch::Response::Buffer #:nodoc:
attr_accessor :ignore_disconnect

def initialize(response)
super(response, SizedQueue.new(10))
super(response, build_queue(self.class.queue_size))
@error_callback = lambda { true }
@cv = new_cond
@aborted = false
Expand Down Expand Up @@ -219,6 +224,10 @@ def each_chunk(&block)
yield str
end
end

def build_queue(queue_size)
queue_size ? SizedQueue.new(queue_size) : Queue.new
end
end

class Response < ActionDispatch::Response #:nodoc: all
Expand Down
3 changes: 3 additions & 0 deletions actionpack/lib/action_controller/test_case.rb
Expand Up @@ -24,6 +24,9 @@ module Live
def new_controller_thread # :nodoc:
yield
end

# Avoid a deadlock from the queue filling up
Buffer.queue_size = nil
end

# ActionController::TestCase will be deprecated and moved to a gem in the future.
Expand Down
17 changes: 17 additions & 0 deletions actionpack/test/controller/live_stream_test.rb
Expand Up @@ -264,6 +264,13 @@ def overfill_buffer_and_die
end
end

def overfill_default_buffer
("a".."z").each do |char|
response.stream.write(char)
end
response.stream.close
end

def ignore_client_disconnect
response.stream.ignore_disconnect = true

Expand Down Expand Up @@ -368,7 +375,15 @@ def test_async_stream
assert t.join(3), "timeout expired before the thread terminated"
end

def test_infinite_test_buffer
get :overfill_default_buffer
assert_equal ("a".."z").to_a.join, response.stream.body
end

def test_abort_with_full_buffer
old_queue_size = ActionController::Live::Buffer.queue_size
ActionController::Live::Buffer.queue_size = 10

@controller.latch = Concurrent::CountDownLatch.new
@controller.error_latch = Concurrent::CountDownLatch.new

Expand All @@ -389,6 +404,8 @@ def test_abort_with_full_buffer
@controller.error_latch.wait
assert_match "Error while streaming", output.rewind && output.read
end
ensure
ActionController::Live::Buffer.queue_size = old_queue_size
end

def test_ignore_client_disconnect
Expand Down

0 comments on commit f6fb700

Please sign in to comment.