Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix: Testing an ActionController::Live controller leads to a deadlock #31938

Closed
wants to merge 2 commits into from
Closed

Fix: Testing an ActionController::Live controller leads to a deadlock #31938

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ebd9150
Use infinite sized queue in rack test context to avoid deadlock situa…
armcburney Feb 8, 2018
cfd64d8
Updated test coupled to queue length in `actionpack/test/controller/l…
armcburney Feb 9, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion actionpack/lib/action_controller/metal/live.rb
View file
Open in desktop
Expand Up @@ -141,7 +141,7 @@ def initialize(response)
@cv = new_cond
@aborted = false
@ignore_disconnect = false
super(response, SizedQueue.new(10))
super(response, Rails.env.test? ? Queue.new : SizedQueue.new(10))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a layering violation for us to be looking at Rails.env here... and within the framework there's nothing special about the name test anyway.

I think it could be worth looking at option 3, though "why a SizedQueue at all?" is a good question. Does checking git blame for the commit that introduced it reveal anything interesting?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change from an infinite queue to a finite one was made in this commit: 37a764b.

The commit message says "use a sized buffer to prevent the queue being too large". Do you have more context @tenderlove? Thank you!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just asked myself why just to change the behaviour just for the test environment. That makes no sense and it is overall dangerous. For example I am faceing the problem in a production environment, see #31200. It crashes the whole app. If you change this behaviour only for testing, the app will still break in production and worse you would not notice it while testing.

But another thing that makes me suspicious is that 37a764b is dating back to 2012. My app was running fine for years with this code until migrating to rails 5. So this can not cause the deadlock alone. Something else must be changed that leads to this issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ran into this same problem. It looks like this is only needed because of the ActionController::Live monkey patch in action_controller/test_case.rb so it seems like it would make sense to have a corresponding monkey patch for this queue. They are coupled together, since that existing patch makes the response synchronous, so an integration test can't consume the queue as it is written to in order to avoid it filling up and deadlocking.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened #41609 which uses an attribute on the class to specify the queue length, so it can be overridden in action_controller/test_case.rb using just an attribute assignment. Hopefully that is clean enough to be accepted.

end

def write(string)
Expand Down
40 changes: 21 additions & 19 deletions actionpack/test/controller/live_stream_test.rb
View file
Open in desktop
Expand Up @@ -330,25 +330,27 @@ def test_async_stream
end

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

capture_log_output do |output|
get :overfill_buffer_and_die, format: "plain"

t = Thread.new(response) { |resp|
resp.await_commit
_, _, body = resp.to_a
body.each do
@controller.latch.wait
body.close
break
end
}

t.join
@controller.error_latch.wait
assert_match "Error while streaming", output.rewind && output.read
Rails.stub(:env, ActiveSupport::StringInquirer.new("production")) do
@controller.latch = Concurrent::CountDownLatch.new
@controller.error_latch = Concurrent::CountDownLatch.new

capture_log_output do |output|
get :overfill_buffer_and_die, format: "plain"

t = Thread.new(response) { |resp|
resp.await_commit
_, _, body = resp.to_a
body.each do
@controller.latch.wait
body.close
break
end
}

t.join
@controller.error_latch.wait
assert_match "Error while streaming", output.rewind && output.read
end
end
end

Expand Down