Don't assume that rack/chunked exists.

[ioquatix]

Rack::Chunked was a bad idea and has been deprecated in Rack 3.0 and will be removed in Rack 3.1. Therefore, don't require that file directly. In Rack 2, it will be autoloaded correctly.

[rafaelfranca]

Would not this change make Rails on Rack 3.0 show a deprecation warning every time this feature is used, and on Rack 3.1 we would have an error? It seems we need to rewrite Streaming to not use Chunked.

[ioquatix]

The deprecation warning will be shown only once (and only if warnings are turned on), and I suggest we rewrite this feature for Rails 8 and leave the current implementation for Rails 7.x. AFAICT, there are no tests for this feature either.

[rafaelfranca]

I suggest we rewrite this feature for Rails 8 and leave the current implementation for Rails 7.x

Why? I don't want to ship code to users that show deprecation. If the framework is using deprecated code, and we can avoid this, we should stop using deprecated code. Even if that means copying the code from chunked to inside Rails while Rack doesn't have an API for it (assuming that is the limitation).

[ioquatix]

Why?

How much feature churn do you want in a minor release? The streaming function will work (as much as it did before), and there is no need to copy the code to retain compatibility. The problem with Rack::Chunked is that it assumes the HTTP/1.1 chunked response can be written to server and it will work. This is not true for Falcon and I don't know if it's true for Puma either. Maybe it worked on Unicorn? I don't think anyone is testing this. If we care about this working (if it ever worked on server X), we should add a small test case to https://github.com/socketry/rack-conform which has (all major servers × versions of rack × integration tests).

We could try to provide a Rack 3 compatible rewrite, but strictly speaking that's introducing new features rather than simply enabling compatibility. If that's what you'd like to do, I suggest we merge this PR, and then make a new feature PR to implement that change.

I'd be happy to work on that after the rack-3 branch is merged.

[simi]

The deprecation warning will be shown only once (and only if warnings are turned on), and I suggest we rewrite this feature for Rails 8 and leave the current implementation for Rails 7.x. AFAICT, there are no tests for this feature either.

Are those warnings turned on by default on new rails app with Rack 3?

[ioquatix]

Yes, IIUC, warn method in Ruby prints to $stderr. I actually don't know if there is a general way to turn it off, but if we were only limited to recent versions of Ruby, we could have written warn "...", category: :deprecations which would only emit if deprecation warnings were turned on.

[ioquatix]

irb(main):002:0> warn "Hello", category: :deprecated
=> nil
irb(main):003:0> Warning[:deprecated] = true
=> true
irb(main):004:0> warn "Hello", category: :deprecated
Hello
=> nil       

Unfortunately not compatible with Ruby 2.7:

irb(main):001:0> warn "Hello", category: :deprecated
Traceback (most recent call last):
	22: from /home/samuel/.gem/ruby/2.7.7/bin/irb:23:in `<main>'
	21: from /home/samuel/.gem/ruby/2.7.7/bin/irb:23:in `load'
	20: from /home/samuel/.gem/ruby/2.7.7/gems/irb-1.6.2/exe/irb:11:in `<top (required)>'
	 3: from (irb):1:in `<main>'
	 2: from /home/samuel/.rubies/ruby-2.7.7/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_warn.rb:19:in `block in <module:Kernel>'
	 1: from /home/samuel/.rubies/ruby-2.7.7/lib/ruby/site_ruby/2.7.0/rubygems/core_ext/kernel_warn.rb:19:in `call'
<internal:warning>:42:in `warn': unknown keyword: :category (ArgumentError)

[ioquatix]

@rafaelfranca what would you like to do here?

cc @tenderlove

[ioquatix]

I thought about this a bit more. We don't have to merge this PR for Rack 3.0, but if we don't the warning will be displayed every time Rails is loaded vs only when the feature is used (via autoload).

That being said, I believe some of the test suites expect no warnings to be printed, so maybe it is required to get the test suite passing completely.

[guilleiguaran]

@ioquatix as @rafaelfranca suggested earlier, can we just bring the relevant code to Rails and then do the required changes to get this working fine with the next version of Rack?

This seems to be passing fine with all relevant tests:

diff --git a/actionpack/lib/action_controller/metal/streaming.rb b/actionpack/lib/action_controller/metal/streaming.rb
index 58a6d9c573..a457ca2d59 100644
--- a/actionpack/lib/action_controller/metal/streaming.rb
+++ b/actionpack/lib/action_controller/metal/streaming.rb
@@ -1,7 +1,5 @@
 # frozen_string_literal: true

-require "rack/chunked"
-
 module ActionController # :nodoc:
   # = Action Controller \Streaming
   #
@@ -208,6 +206,35 @@ module ActionController # :nodoc:
   # * https://www.phusionpassenger.com/docs/references/config_reference/nginx/#passenger_buffer_response
   #
   module Streaming
+    class Body # :nodoc:
+      TERM = "\r\n"
+      TAIL = "0#{TERM}"
+
+      # Store the response body to be chunked.
+      def initialize(body)
+        @body = body
+      end
+
+      # For each element yielded by the response body, yield
+      # the element in chunked encoding.
+      def each(&block)
+        term = TERM
+        @body.each do |chunk|
+          size = chunk.bytesize
+          next if size == 0
+
+          yield [size.to_s(16), term, chunk.b, term].join
+        end
+        yield TAIL
+        yield term
+      end
+
+      # Close the response body if the response body supports it.
+      def close
+        @body.close if @body.respond_to?(:close)
+      end
+    end
+
     private
       # Set proper cache control and transfer encoding when streaming
       def _process_options(options)
@@ -226,7 +253,7 @@ def _process_options(options)
       # Call render_body if we are streaming instead of usual +render+.
       def _render_template(options)
         if options.delete(:stream)
-          Rack::Chunked::Body.new view_renderer.render_body(view_context, options)
+          Body.new view_renderer.render_body(view_context, options)
         else
           super
         end

[ioquatix]

Based on my testing, it's okay.

[ioquatix]

Closing as no longer needed.