rails
Utilize Rack 3 streaming.
Motivation / Background
See https://github.com/rails/rails/issues/52066 for context.
Rack 3 introduced several features to enhance how "streaming" responses are handled.
- The presence of
body#to_aryindicates that a response can be buffered (i.e. converted to an Array of String instances, without affecting the application behaviour). #eachconsumes the body, i.e. yields successive chunks that can be sent incrementally using chunked encoding. This is also somewhat valid for Rack 2.- Instead of responding to
#each, a body can respond to#call, which gets invoked like a partial hijack and has identical semantics.
Rails is still generating transfer-encoding: chunked encoded responses as a Rack body, which can cause problems:
- Other Rack middleware may not be able to correctly manipulate the response body because it is encoded.
- e.g.
rack-cachehas been mentioned. However, it's absolutely possible to cache streaming responses and this is just a limitation of the current implementation of Rails.
- e.g.
transfer-encodingis not supported by HTTP/2 as every response is effectively considered chunked and the wire format uses binary framing rather than length-prefixed chunked encoding. The Rails implementation is more limited than necessary in this regard.- The complexity of the implementation is unnecessarily high and specific to HTTP/1. In addition, this complexity is already handled by the servers in a protocol-appropriate way.
This change removes the explicit transfer encoding and instead relies on servers to correctly encode the response.
Checklist
Before submitting the PR make sure the following are checked:
- [x] This Pull Request is related to one change. Unrelated changes should be opened in separate PRs.
- [x] Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex:
[Fix #issue-number] - [x] Tests are added or updated if you fix a bug or add a feature.
- [ ] CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.
existing applications will need to ensure they add chunked responses on streaming
Do you mind elaborating on what you mean here? I don't think existing applications need to do anything differently?
Do you mean existing servers? If that's what you meant, that's already how it works - at least they can decide according to the connection - be it HTTP/1.0, HTTP/1.1 or HTTP/2+. I've just added a specific test for this to rack-conform: https://github.com/socketry/rack-conform/pull/20 (ignore the puma failures, they are unrelated).
Do you mind elaborating on what you mean here? I don't think existing applications need to do anything differently?
Existing applications that use this code path will expect that their responses have chunked transfer encoding. Since this class is removed, now the response is not chunked. How is the downstream web server supposed to know to use chunked responses? I would guess the answer is that they check to_ary but also check whether or not there is a transfer-encoding header. Otherwise this codepath wouldn't work on webservers today. I haven't done the legwork to verify any of this though.
Additionally, it looks like we do something special when the request is HTTP/1.0, but the test for that was just deleted?
I don't follow how this is a backwards compatible change, and I'd like an explanation so I can better understand how this won't break existing apps.
Existing applications that use this code path will expect that their responses have chunked transfer encoding.
We do not provide absolute requirements in Rack for how a given response should be processed by the server and handled by the client. Rack itself does not have the capacity to test for this either - nor should it.
Rack is loosely based on the CGI model as specified by RFC3875. One can look at the Rack server as being a proxy (intermediary) between the Client Browser and the Rack middleware/application and thus it should be subject to the requirements outlined in RFC9110 - in other words, if the client was HTTP/2 and the Rack application is CGI (loosely HTTP/1.1), it should consume the HTTP/1.1 response and map it to valid HTTP/2 semantics.
sequenceDiagram
participant Client as Client Browser
participant Server as Rack Web Server
participant Middleware as Rack Middleware
participant App as Application
Client->>Server: Request (HTTP/1.0, HTTP/1.1, HTTP/2, HTTP/3)
Server->>Middleware: Invoke Middleware (CGI)
Middleware->>App: Forward Request (CGI)
App-->>Middleware: Response (CGI)
Middleware-->>Server: Response (CGI)
Server-->>Client: Response (HTTP/1.0, HTTP/1.1, HTTP/2, HTTP/3)
When I started working on introducing support for HTTP/2 to Rack, I decided that this approach was significantly more complex and prevented evolution of the Rack protocol - IOW, we'd be stuck on CGI forever and all the warts and problems it has.
Given that our dependent on CGI seems like a convenience rather than an absolute requirement, the evolution of Rack to support HTTP/2 has been one where I've tried to push us gracefully towards a common middle-ground, in other words, Rack is slowly moving from RFC3875 to RFC9110 (linked above respectively) - IOW Rack follows the HTTP semantics rather than a specific version of HTTP.
The consequence of this, is that we should avoid doing HTTP version specific logic within Rack if possible, and instead Rack should provide a common set of semantics (like rack.protocol for handling upgrades). Those semantics should have a clear mapping to the underlying protocol features if possible but the actual implementation is left up to the servers.
To give one objective example, it would be entirely possible for Falcon to see a transfer-encoding: chunked response, decode that response, and rewrite it for HTTP/2. But such an approach is extremely inefficient, incurring not only the original encoding, but a subsequent decoding and re-encoding step. Therefore, Rack provides an abstract semantic relating to #to_ary and #each to specify 1/ here is how to provide a response that can be buffered easily and 2/ here is how to provide a response that can be incrementally sent to the client.
Since this class is removed, now the response is not chunked. How is the downstream web server supposed to know to use chunked responses?
The server should not be forced to use a specific encoding or not. It should be up to the server to decide what is most appropriate for the client that's connecting. I don't think this logic should be implemented in the application layer (or framework layer if you consider Rails). It seems like a layering violation in the implementation. The application should not need to care about what kind of client has connected or what is the best kind of response to create. Rack should provide the appropriate abstractions to ensure that applications can focus on business logic, not network protocols.
With that being said, there is an expectation that a response body that does not respond to #to_ary (can't be buffered) and responds to #each should be sent incrementally. Since there was no previous testing for this, I created rack-conform to test the interactions between client browsers and rack applications. This is an end-to-end integration test, confirming the actual client side behaviour of a given server + application. As shown on this PR, we can confirm that servers are indeed working as intended. However, sometimes the buffering strategy is different - i.e. it's not always guaranteed that servers will send chunks independently - and I think that's perfectly okay and within the boundaries of what a server could decide to do.
Otherwise this codepath wouldn't work on webservers today. I haven't done the legwork to verify any of this though.
So to be crystal clear, I believe I have done the leg work needed to verify that, and you can see it for yourself. Feel free to introduce additional tests to the above test suite if you want to see their behaviour across a wide range (all mainstream?) of servers. It would be great to have additional eyeballs on this.
Additionally, it looks like we do something special when the request is HTTP/1.0, but the test for that was just deleted?
As discussed above, this is a concern for the server, not the application code. The test was deleted as it shouldn't be something that the streaming tests should be concerned with. In fact, it's quite okay to stream a response in HTTP/1.0, and the fact Rails was skipping streaming on HTTP/1.0 was an artificial limitation on the performance (although not one that should be relevant in any real world situation today).
Separately, it would be good to write additional tests that confirm the chunks generated by rails - Rack::Test correctly normalises the response whether it's streaming or not - so that tests work the same irrespective of what a middleware/server decided to do - and I don't think we should change that. It would be better to have unit tests that assert that invoking a given controller produces a specific list of chunks.
I don't follow how this is a backwards compatible change, and I'd like an explanation so I can better understand how this won't break existing apps.
It's never possible to know for certain that a change is or isn't backwards compatible as I'm sure you well know. However, despite the tests breaking in Rails, streaming responses were still working correctly. The problem was the specificity of the test suite - and that's something I have a reasonably strong opinion on - the tests are operating at the wrong level to actually confirm streaming is occurring. The best you could do is confirm that the controller is returning a body with multiple chunks. But this test is trying to confirm the wire format of the response, using a mechanism that was never designed to be used that way.
As a counter point to compatibility breakage - this PR actually makes a wider range of streaming opportunities possible - e.g. HTTP/1.0 and HTTP/2+ are now correctly supported - by using the correct "abstract semantics" provided by Rack, we can provide servers the opportunity to stream the response - and that's the best we can do at the application layer - and that's what we should test for.
Hello :wave:
I'm not 100% sure this change is the reason but unfortunately you were probably right @tenderlove, this change or some other streaming related change in 8.0 did break streaming compatibility for me when upgrading from 7.2 to 8.0.1
I've been using streaming in rails since 2015 with https://github.com/jarthod/render-later and after the 7.2 to 8.0.1 update I started getting this exception on my streaming pages:
curl http://localhost:3000
Puma caught this error: undefined method `call' for nil (NoMethodError)
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/actionview-8.0.1/lib/action_view/renderer/streaming_template_renderer.rb:23:in `rescue in each'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/actionview-8.0.1/lib/action_view/renderer/streaming_template_renderer.rb:19:in `each'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/actionpack-8.0.1/lib/action_dispatch/http/response.rb:111:in `to_ary'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/actionpack-8.0.1/lib/action_dispatch/http/response.rb:523:in `to_ary'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/rack-3.1.10/lib/rack/body_proxy.rb:51:in `method_missing'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/rack-3.1.10/lib/rack/body_proxy.rb:51:in `method_missing'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/rack-3.1.10/lib/rack/body_proxy.rb:51:in `method_missing'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/rack-3.1.10/lib/rack/body_proxy.rb:51:in `method_missing'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/puma-6.6.0/lib/puma/request.rb:187:in `prepare_response'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/puma-6.6.0/lib/puma/request.rb:132:in `handle_request'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/puma-6.6.0/lib/puma/server.rb:472:in `process_client'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/puma-6.6.0/lib/puma/server.rb:254:in `block in run'
/home/bigbourin/.rbenv/versions/3.3.5/lib/ruby/gems/3.3.0/gems/puma-6.6.0/lib/puma/thread_pool.rb:167:in `block in spawn_thread'
I tried removing all my Rack streaming specific patches and disabling all my middlewares of course but it didn't change much. I was already on Rack 3.1 so this is not linked to the rack 2 → 3 upgrade, which I already did a while back.
The problem can easily be reproduced from the small dummy test rails app I created in https://github.com/jarthod/render-later repo for automated testing:
cd test/dummy
bundle install --gemfile=../gemfiles/rails-8.0
bundle exec --gemfile=../gemfiles/rails-8.0 rails s
# in another terminal:
curl http://localhost:3000
# same error shown above
The good news at least (IMO) is that I can't reproduce with Rails main, so I guess the problem have already been fixed in another comit (or it's a very specific edge-case I'm having here ?) So I'm not gonna spend more hours investigating this on my side and I'm gonna wait for future 8.0.x release, hopefully in 8.0.2 it'll be fixed.
But as I couldn't find any reference to this bug online and in order to save time for people who may encounter the same exception when they upgrade, I though I would share this here to save them a bit of time and effort in finding a solution.
Here is the test matrix showing the failures on Rails 8.0.1 only: https://github.com/jarthod/render-later/actions/runs/13290274804/job/37109071845 (it's not easy to see the exception here though after capybara, that's why I show the simpler manual curl test above)
If you are able to reproduce it, could you try bisecting the good commit from main? We might be able to backport it.
The issue was fixed in https://github.com/rails/rails/pull/51508 and I asked repeatedly for this to be merged before Rails 8 was released but unfortunately it was not.
@ioquatix thanks, I see this comit was merged in rails main on November 1st, 2024 though, and Rails 8.0.1 was released on December 13, was it not included ? (or maybe the issue is something else)
Rails main is already 8.1.0-alpha since october: https://github.com/rails/rails/blob/main/RAILS_VERSION
@p8 Ah ok thanks! Well then if #51508 could be backported to 8.0.2+ that would be a good idea IMO, not just for me but to avoid breaking streaming for people upgrading to 8.0 between 7.2 and 8.1. If this is something I can help with let me know!
@jarthod Sorry for the late reply. To get this backported, open a new issue based on your comment: https://github.com/rails/rails/pull/52094#issuecomment-2654282368 .