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.

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

actionpack: Use an infinite sized queue in testing ActionController::Live #41609

Merged
merged 1 commit into from Jun 23, 2021

Conversation

@dylanahsmith
Copy link
Contributor

@dylanahsmith dylanahsmith commented Mar 3, 2021

Fixes #31813

Problem Summary

ActionController::Live normally processes the request in another thread, however, a monkey patch in [ActionController::Live monkey patch in action_controller/test_case.rb] forces it to instead get processed in the same thread. Since the writes go to a SizedQueue.new(10) in ActionController::Live::Buffer, a test will deadlock if the controller makes more than 10 writes.

Solution

The fix is similar to #31938, we use an infinitely sized Queue.new for testing. This PR just does this more cleanly by using an attribute on the class to configure the queue size, which allows both the test and normal behaviour to be tested cleanly.

@rails-bot
Copy link

@rails-bot rails-bot bot commented Jun 1, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Jun 1, 2021
zzak
zzak approved these changes Jun 2, 2021
@rails-bot rails-bot bot removed the stale label Jun 2, 2021
@zzak
Copy link
Member

@zzak zzak commented Jun 2, 2021

@dylanahsmith This LGTM, can you rebase the CHANGELOG?

@dylanahsmith dylanahsmith force-pushed the fix-live-stream-test branch from a5a29e4 to 93e0421 Jun 2, 2021
…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.
@dylanahsmith dylanahsmith force-pushed the fix-live-stream-test branch from 93e0421 to f6fb700 Jun 3, 2021
@dylanahsmith
Copy link
Contributor Author

@dylanahsmith dylanahsmith commented Jun 3, 2021

I've rebased the PR and handled the changelog conflict.

@zzak zzak added the ready label Jun 4, 2021
@rafaelfranca rafaelfranca merged commit 3a98e7d into rails:main Jun 23, 2021
4 checks passed
rafaelfranca added a commit that referenced this issue Jun 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants