Skip to content

Commit

Permalink
Don't stream redirect controller responses
Browse files Browse the repository at this point in the history
They don't need to be streamed and by including
ActiveStorage::Streaming, they spin up new threads which can cause
problems with connection pools as detailed in rails#44242
  • Loading branch information
lukel97 committed Jan 26, 2022
1 parent d5b65c0 commit 7e61f80
Show file tree
Hide file tree
Showing 4 changed files with 12 additions and 1 deletion.
8 changes: 8 additions & 0 deletions activestorage/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,11 @@
* Don't stream responses in redirect mode

Previously, both redirect mode and proxy mode streamed their
responses which caused a new thread to be created, and could end
up leaking connections in the connection pool. But since redirect
mode doesn't actually send any data, it doesn't need to be
streamed.

*Luke Lau*

Please check [7-0-stable](https://github.com/rails/rails/blob/7-0-stable/activestorage/CHANGELOG.md) for previous changes.
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

# The base class for all Active Storage controllers.
class ActiveStorage::BaseController < ActionController::Base
include ActiveStorage::SetCurrent, ActiveStorage::Streaming
include ActiveStorage::SetCurrent

protect_from_forgery with: :exception

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
# {Authenticated Controllers}[https://guides.rubyonrails.org/active_storage_overview.html#authenticated-controllers].
class ActiveStorage::Blobs::ProxyController < ActiveStorage::BaseController
include ActiveStorage::SetBlob
include ActiveStorage::Streaming

def show
if request.headers["Range"].present?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
# require a higher level of protection consider implementing
# {Authenticated Controllers}[https://guides.rubyonrails.org/active_storage_overview.html#authenticated-controllers].
class ActiveStorage::Representations::ProxyController < ActiveStorage::Representations::BaseController
include ActiveStorage::Streaming

def show
http_cache_forever public: true do
send_blob_stream @representation.image, disposition: params[:disposition]
Expand Down

0 comments on commit 7e61f80

Please sign in to comment.