Skip to content

Don't buffer things that shouldn't be buffered.#51508

Merged
matthewd merged 3 commits into
rails:mainfrom
ioquatix:actiondispatch-response-body-to_ary
Nov 1, 2024
Merged

Don't buffer things that shouldn't be buffered.#51508
matthewd merged 3 commits into
rails:mainfrom
ioquatix:actiondispatch-response-body-to_ary

Conversation

@ioquatix

@ioquatix ioquatix commented Apr 6, 2024

Copy link
Copy Markdown
Contributor

Assigning ActionDispatch::Response to self.response causes the response body to be buffered (and potentially evaluated multiple times).

class StreamingController < ApplicationController
  def simple
    body = Enumerator.new do |enumerator|
      enumerator << "." * 1024
      
      100.times do |i|
        enumerator << "This is line #{i}\n"
        sleep 0.1
      end
    end

    # Works, puma, falcon, Rails 7.1
    # self.response = Rack::Response[200, {"content-type" => "text/plain"}, body]

    # Doesn't work because the response is buffered:
    self.response = ActionDispatch::Response.new(200, {"content-type" => "text/plain"}, body)
  end
end

In both Puma and Falcon, the response is buffered without this change.

cc @willcosgrove

@rails-bot rails-bot Bot added the actionpack label Apr 6, 2024
@ioquatix ioquatix force-pushed the actiondispatch-response-body-to_ary branch from 46caa06 to 39e1787 Compare April 6, 2024 09:57
@ioquatix ioquatix force-pushed the actiondispatch-response-body-to_ary branch from c6eeefd to 2add03d Compare August 9, 2024 22:07
@rafaelfranca

Copy link
Copy Markdown
Member

Many tests are broken

@ioquatix ioquatix force-pushed the actiondispatch-response-body-to_ary branch from 2add03d to eea9b38 Compare August 12, 2024 21:13
@ioquatix

Copy link
Copy Markdown
Contributor Author

Thanks, let me rebase and update again, there has been a bit of churn.

@ioquatix

Copy link
Copy Markdown
Contributor Author

There was a good discussion about some of the previous (related) changes here: #24029

@ioquatix

Copy link
Copy Markdown
Contributor Author

@rafaelfranca I think we should merge #52253 first as it fixes some of the core issues with the Buffer class and streaming responses. Once that's done, I'll rework this PR on top of that.

@ioquatix ioquatix force-pushed the actiondispatch-response-body-to_ary branch from eea9b38 to 97b96df Compare August 13, 2024 00:04
@rails-bot rails-bot Bot added the actionview label Aug 13, 2024
@ioquatix ioquatix marked this pull request as draft August 13, 2024 00:04
@ioquatix ioquatix force-pushed the actiondispatch-response-body-to_ary branch 2 times, most recently from a1f29f6 to d486784 Compare August 13, 2024 01:09
@ioquatix

ioquatix commented Aug 13, 2024

Copy link
Copy Markdown
Contributor Author

I've rebased this on #52253 which we should merge first before this PR.

@ioquatix ioquatix force-pushed the actiondispatch-response-body-to_ary branch 3 times, most recently from 42c6548 to 2f10495 Compare August 15, 2024 03:43
@ioquatix

Copy link
Copy Markdown
Contributor Author

I've rebased this on #52253 again.

@ioquatix ioquatix force-pushed the actiondispatch-response-body-to_ary branch from 2f10495 to a8481b2 Compare September 27, 2024 01:48
@ioquatix ioquatix force-pushed the actiondispatch-response-body-to_ary branch 2 times, most recently from c69f4a1 to a9b2da0 Compare November 1, 2024 06:26
@ioquatix ioquatix force-pushed the actiondispatch-response-body-to_ary branch from a9b2da0 to 2bc2263 Compare November 1, 2024 06:28
@ioquatix ioquatix marked this pull request as ready for review November 1, 2024 06:29
@matthewd matthewd merged commit a7c8a20 into rails:main Nov 1, 2024
@ioquatix ioquatix deleted the actiondispatch-response-body-to_ary branch November 1, 2024 09:05
@ioquatix

ioquatix commented Nov 1, 2024

Copy link
Copy Markdown
Contributor Author

Thanks @matthewd, really appreciate all your detailed feedback!

@ioquatix ioquatix mentioned this pull request Feb 13, 2025
4 tasks
zzak pushed a commit to zzak/rails that referenced this pull request Mar 25, 2025
…body-to_ary

Don't buffer things that shouldn't be buffered.
@joshuay03

Copy link
Copy Markdown
Contributor

Should this have a changelog entry?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants