Re: Rack 3 Compatibility

[Jeremy Evans <code@jeremyevans.net>]

On 06/02 12:00, Eric Wong wrote:
> Jeremy Evans <code@jeremyevans.net> wrote:
> > This takes Eric's patch from December 25, 2022, and includes all
> > necessary test fixes to allow Unicorn tests to pass with both
> > Rack 3 and Rack 2 (and probably Rack 1). It includes a test fix for
> > newer curl versions and an OpenBSD test fix.
> > 
> > Hopefully this is acceptable and Unicorn 6.2 can be released with Rack 3
> > support.  If further fixes are needed, I'm happy to work on them.
> 
> Isn't a chunk replacement needed for Rack 3.1, also?
> I dunno if I missed anything else in Rack 3.x; and don't want to
> make too many releases if we can do 3.0 and 3.1 in one go.

We deprecated Rack::Chunked in Rack 3.0 and plan to remove it in Rack
3.1. I agree it would be best to deal with this now, I just wasn't sure
how you wanted to handle it.  Your patch below to deal with it at the
server level looks good, though it doesn't appear to remove the Chunked
usage at line 69 of unicorn.rb.  I recommend that also be removed.

Thanks,
Jeremy

> -------8<-------
> Subject: [PATCH] chunk unterminated HTTP/1.1 responses
> 
> Rack::Chunked will be gone in Rack 3.1, so provide a
> non-middleware fallback which takes advantage of IO#write
> supporting multiple arguments in Ruby 2.5+.
> 
> We still need to support Ruby 2.4, at least, since Rack 3.0
> does.  So a new (GC-unfriendly) Unicorn::WriteSplat module now
> exists for Ruby <= 2.4 users.
> ---
>  ext/unicorn_http/unicorn_http.rl | 11 +++++++++++
>  lib/unicorn.rb                   |  4 ++--
>  lib/unicorn/http_response.rb     | 21 ++++++++++++++++++++-
>  lib/unicorn/socket_helper.rb     | 18 ++++++++++++++++--
>  lib/unicorn/write_splat.rb       |  7 +++++++
>  test/unit/test_server.rb         |  2 +-
>  6 files changed, 57 insertions(+), 6 deletions(-)
>  create mode 100644 lib/unicorn/write_splat.rb
> 
> diff --git a/ext/unicorn_http/unicorn_http.rl b/ext/unicorn_http/unicorn_http.rl
> index ba23438..c339024 100644
> --- a/ext/unicorn_http/unicorn_http.rl
> +++ b/ext/unicorn_http/unicorn_http.rl
> @@ -28,6 +28,7 @@ void init_unicorn_httpdate(void);
>  #define UH_FL_TO_CLEAR 0x200
>  #define UH_FL_RESSTART 0x400 /* for check_client_connection */
>  #define UH_FL_HIJACK 0x800
> +#define UH_FL_RES_CHUNK_OK (1U << 12)
>  
>  /* all of these flags need to be set for keepalive to be supported */
>  #define UH_FL_KEEPALIVE (UH_FL_KAVERSION | UH_FL_REQEOF | UH_FL_HASHEADER)
> @@ -158,6 +159,7 @@ http_version(struct http_parser *hp, const char *ptr, size_t len)
>    if (CONST_MEM_EQ("HTTP/1.1", ptr, len)) {
>      /* HTTP/1.1 implies keepalive unless "Connection: close" is set */
>      HP_FL_SET(hp, KAVERSION);
> +    HP_FL_SET(hp, RES_CHUNK_OK);
>      v = g_http_11;
>    } else if (CONST_MEM_EQ("HTTP/1.0", ptr, len)) {
>      v = g_http_10;
> @@ -801,6 +803,14 @@ static VALUE HttpParser_keepalive(VALUE self)
>    return HP_FL_ALL(hp, KEEPALIVE) ? Qtrue : Qfalse;
>  }
>  
> +/* :nodoc: */
> +static VALUE chunkable_response_p(VALUE self)
> +{
> +  struct http_parser *hp = data_get(self);
> +
> +  return HP_FL_ALL(hp, RES_CHUNK_OK) ? Qtrue : Qfalse;
> +}
> +
>  /**
>   * call-seq:
>   *    parser.next? => true or false
> @@ -981,6 +991,7 @@ void Init_unicorn_http(void)
>    rb_define_method(cHttpParser, "content_length", HttpParser_content_length, 0);
>    rb_define_method(cHttpParser, "body_eof?", HttpParser_body_eof, 0);
>    rb_define_method(cHttpParser, "keepalive?", HttpParser_keepalive, 0);
> +  rb_define_method(cHttpParser, "chunkable_response?", chunkable_response_p, 0);
>    rb_define_method(cHttpParser, "headers?", HttpParser_has_headers, 0);
>    rb_define_method(cHttpParser, "next?", HttpParser_next, 0);
>    rb_define_method(cHttpParser, "buf", HttpParser_buf, 0);
> diff --git a/lib/unicorn.rb b/lib/unicorn.rb
> index 1a50631..8b1cda7 100644
> --- a/lib/unicorn.rb
> +++ b/lib/unicorn.rb
> @@ -75,8 +75,8 @@ def self.builder(ru, op)
>  
>        # return value, matches rackup defaults based on env
>        # Unicorn does not support persistent connections, but Rainbows!
> -      # and Zbatery both do.  Users accustomed to the Rack::Server default
> -      # middlewares will need ContentLength/Chunked middlewares.
> +      # does.  Users accustomed to the Rack::Server default
> +      # middlewares will need ContentLength middleware.
>        case ENV["RACK_ENV"]
>        when "development"
>        when "deployment"
> diff --git a/lib/unicorn/http_response.rb b/lib/unicorn/http_response.rb
> index 19469b4..342dd0b 100644
> --- a/lib/unicorn/http_response.rb
> +++ b/lib/unicorn/http_response.rb
> @@ -35,11 +35,12 @@ def append_header(buf, key, value)
>    def http_response_write(socket, status, headers, body,
>                            req = Unicorn::HttpRequest.new)
>      hijack = nil
> -
> +    do_chunk = false
>      if headers
>        code = status.to_i
>        msg = STATUS_CODES[code]
>        start = req.response_start_sent ? ''.freeze : 'HTTP/1.1 '.freeze
> +      term = false
>        buf = "#{start}#{msg ? %Q(#{code} #{msg}) : status}\r\n" \
>              "Date: #{httpdate}\r\n" \
>              "Connection: close\r\n"
> @@ -47,6 +48,12 @@ def http_response_write(socket, status, headers, body,
>          case key
>          when %r{\A(?:Date|Connection)\z}i
>            next
> +        when %r{\AContent-Length\z}i
> +          append_header(buf, key, value)
> +          term = true
> +        when %r{\ATransfer-Encoding\z}i
> +          append_header(buf, key, value)
> +          term = true if /\bchunked\b/i === value # value may be Array :x
>          when "rack.hijack"
>            # This should only be hit under Rack >= 1.5, as this was an illegal
>            # key in Rack < 1.5
> @@ -55,12 +62,24 @@ def http_response_write(socket, status, headers, body,
>            append_header(buf, key, value)
>          end
>        end
> +      if !hijack && !term && req.chunkable_response?
> +        do_chunk = true
> +        buf << "Transfer-Encoding: chunked\r\n".freeze
> +      end
>        socket.write(buf << "\r\n".freeze)
>      end
>  
>      if hijack
>        req.hijacked!
>        hijack.call(socket)
> +    elsif do_chunk
> +      begin
> +        body.each do |b|
> +          socket.write("#{b.bytesize.to_s(16)}\r\n", b, "\r\n".freeze)
> +        end
> +      ensure
> +        socket.write("0\r\n\r\n".freeze)
> +      end
>      else
>        body.each { |chunk| socket.write(chunk) }
>      end
> diff --git a/lib/unicorn/socket_helper.rb b/lib/unicorn/socket_helper.rb
> index 8a6f6ee..4ae4c85 100644
> --- a/lib/unicorn/socket_helper.rb
> +++ b/lib/unicorn/socket_helper.rb
> @@ -15,6 +15,20 @@ def kgio_tryaccept # :nodoc:
>      end
>    end
>  
> +  if IO.instance_method(:write).arity # Ruby <= 2.4
> +    require 'unicorn/write_splat'
> +    UNIXClient = Class.new(Kgio::Socket) # :nodoc:
> +    class UNIXSrv < Kgio::UNIXServer # :nodoc:
> +      include Unicorn::WriteSplat
> +      def kgio_tryaccept # :nodoc:
> +        super(UNIXClient)
> +      end
> +    end
> +    TCPClient.__send__(:include, Unicorn::WriteSplat)
> +  else # Ruby 2.5+
> +    UNIXSrv = Kgio::UNIXServer
> +  end
> +
>    module SocketHelper
>  
>      # internal interface
> @@ -135,7 +149,7 @@ def bind_listen(address = '0.0.0.0:8080', opt = {})
>          end
>          old_umask = File.umask(opt[:umask] || 0)
>          begin
> -          Kgio::UNIXServer.new(address)
> +          UNIXSrv.new(address)
>          ensure
>            File.umask(old_umask)
>          end
> @@ -203,7 +217,7 @@ def server_cast(sock)
>          Socket.unpack_sockaddr_in(sock.getsockname)
>          TCPSrv.for_fd(sock.fileno)
>        rescue ArgumentError
> -        Kgio::UNIXServer.for_fd(sock.fileno)
> +        UNIXSrv.for_fd(sock.fileno)
>        end
>      end
>  
> diff --git a/lib/unicorn/write_splat.rb b/lib/unicorn/write_splat.rb
> new file mode 100644
> index 0000000..7e6e363
> --- /dev/null
> +++ b/lib/unicorn/write_splat.rb
> @@ -0,0 +1,7 @@
> +# -*- encoding: binary -*-
> +# compatibility module for Ruby <= 2.4, remove when we go Ruby 2.5+
> +module Unicorn::WriteSplat # :nodoc:
> +  def write(*arg) # :nodoc:
> +    super(arg.join(''))
> +  end
> +end
> diff --git a/test/unit/test_server.rb b/test/unit/test_server.rb
> index 98e85ab..cea9791 100644
> --- a/test/unit/test_server.rb
> +++ b/test/unit/test_server.rb
> @@ -196,7 +196,7 @@ def test_client_shutdown_writes
>      # continue to process our request and never hit EOFError on our sock
>      sock.shutdown(Socket::SHUT_WR)
>      buf = sock.read
> -    assert_equal 'hello!\n', buf.split(/\r\n\r\n/).last
> +    assert_match %r{\bhello!\\n\b}, buf.split(/\r\n\r\n/).last
>      next_client = Net::HTTP.get(URI.parse("http://127.0.0.1:#@port/"))
>      assert_equal 'hello!\n', next_client
>      lines = File.readlines("test_stderr.#$$.log")