Fix: Testing an ActionController::Live controller leads to a deadlock
#31938
Conversation
…tion when data is never consumed from the Action::Dispatch::Response::Buffer.
There was a problem hiding this comment.
Hello and welcome @AndrewMcBurney! Thanks for working on this.
It looks like the change broke the Travis build https://travis-ci.org/rails/rails/jobs/339122934#L1190 - can you take a look?
| # http://ruby-doc.org/stdlib-2.0.0/libdoc/thread/rdoc/SizedQueue.html#method-i-push | ||
| # | ||
| # Thus, use an infinite sized Queue in rack test contexts to work around | ||
| # the issue where data is never consumed from the queue. |
There was a problem hiding this comment.
I think we should delete the documentation. We don't usually comment like this as it makes more sense to put the comment in the commit message. Can you move this there?
…ive_stream_test.rb`, and removed superfluous comment in `actionpack/lib/action_controller/metal/live.rb`.
| @@ -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)) | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Use an infinite sized queue in rack test context to avoid deadlock situation on insertion into a full queue when data is never consumed from the
Action::Dispatch::Response::Buffer.Summary
Hi rails team!
This is my first time contributing to rails. I saw issue #31200, and the subsequent follow-up issue #31813, and thought it would be a good first problem to tackle.
I implemented the second tentative solution listed in #31813, where an infinite queue is used for streaming in a rack test context.
If the team believes it's better to proceed with the third solution (consuming the response in async) over the second one, I'd be happy to try and tackle it and would appreciate any guidance or advice from the team.
Other Information
Just for my understanding (and this may be a silly question, so I apologize in advance if it is). Why was a
SizedQueueused over a regularQueue? Was the limit of10chosen arbitrarily, or was it chosen for a specific reason?Thank you! 🙂