Re: [PATCH] deflater: remove "deflate" encoding support

[James Tucker <jftucker@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 8139 bytes --]

Can you explain the cache hit rate assertions you open with? Sorry for evil
post, mobile client.

On Jul 26, 2016 7:10 PM, "Eric Wong" <e@80x24.org> wrote:

> This will improve cache hit rates and reduce caching overhead at
> small expense of increased header overhead for some user agents.
> For reference, Varnish cache supports only gzip as well:
>
>         https://www.varnish-cache.org/docs/4.1/phk/gzip.html
>
> In the past, "deflate" encoding was more likely to trigger
> user-agent bugs, as noted in the comments removed with this
> change as well as the Varnish documentation referenced above.
> ---
>   The following changes since commit
> 25a549883b85fb33970b4a1530a365c0c9e51f95:
>
>     bumping to 2.0.1 to work around Rack (2016-06-30 10:33:09 -0700)
>
>   are available in the git repository at:
>
>     git://80x24.org/rack.git no-deflate
>
>   for you to fetch changes up to d6380043a8953dca63743c947c8027f465d29a5d:
>
>     deflater: remove "deflate" encoding support (2016-07-26 23:14:55 +0000)
>
>   ----------------------------------------------------------------
>   Eric Wong (1):
>         deflater: remove "deflate" encoding support
>
>  lib/rack/deflater.rb  | 37 +------------------------------------
>  test/spec_deflater.rb | 38 ++++++++++++++++++++++++--------------
>  2 files changed, 25 insertions(+), 50 deletions(-)
>
> diff --git a/lib/rack/deflater.rb b/lib/rack/deflater.rb
> index 62a1124..79e39f5 100644
> --- a/lib/rack/deflater.rb
> +++ b/lib/rack/deflater.rb
> @@ -8,7 +8,6 @@ module Rack
>    # Currently supported compression algorithms:
>    #
>    #   * gzip
> -  #   * deflate
>    #   * identity (no transformation)
>    #
>    # The middleware automatically detects when compression is supported
> @@ -41,7 +40,7 @@ module Rack
>
>        request = Request.new(env)
>
> -      encoding = Utils.select_best_encoding(%w(gzip deflate identity),
> +      encoding = Utils.select_best_encoding(%w(gzip identity),
>                                              request.accept_encoding)
>
>        # Set the Vary HTTP header.
> @@ -57,10 +56,6 @@ module Rack
>          mtime = headers.key?("Last-Modified") ?
>            Time.httpdate(headers["Last-Modified"]) : Time.now
>          [status, headers, GzipStream.new(body, mtime)]
> -      when "deflate"
> -        headers['Content-Encoding'] = "deflate"
> -        headers.delete(CONTENT_LENGTH)
> -        [status, headers, DeflateStream.new(body)]
>        when "identity"
>          [status, headers, body]
>        when nil
> @@ -101,36 +96,6 @@ module Rack
>        end
>      end
>
> -    class DeflateStream
> -      DEFLATE_ARGS = [
> -        Zlib::DEFAULT_COMPRESSION,
> -        # drop the zlib header which causes both Safari and IE to choke
> -        -Zlib::MAX_WBITS,
> -        Zlib::DEF_MEM_LEVEL,
> -        Zlib::DEFAULT_STRATEGY
> -      ]
> -
> -      def initialize(body)
> -        @body = body
> -        @closed = false
> -      end
> -
> -      def each
> -        deflator = ::Zlib::Deflate.new(*DEFLATE_ARGS)
> -        @body.each { |part| yield deflator.deflate(part,
> Zlib::SYNC_FLUSH) }
> -        yield fin = deflator.finish
> -      ensure
> -        deflator.finish unless fin
> -        deflator.close
> -      end
> -
> -      def close
> -        return if @closed
> -        @closed = true
> -        @body.close if @body.respond_to?(:close)
> -      end
> -    end
> -
>      private
>
>      def should_deflate?(env, status, headers, body)
> diff --git a/test/spec_deflater.rb b/test/spec_deflater.rb
> index ba7ec5d..0f27c85 100644
> --- a/test/spec_deflater.rb
> +++ b/test/spec_deflater.rb
> @@ -81,13 +81,22 @@ describe Rack::Deflater do
>      yield(status, headers, body) if block_given?
>    end
>
> +  # automatic gzip detection (streamable)
> +  def auto_inflater
> +    Zlib::Inflate.new(32 + Zlib::MAX_WBITS)
> +  end
> +
> +  def deflate_or_gzip
> +    {'deflate, gzip' => 'gzip'}
> +  end
> +
>    it 'be able to deflate bodies that respond to each' do
>      app_body = Object.new
>      class << app_body; def each; yield('foo'); yield('bar'); end; end
>
> -    verify(200, 'foobar', 'deflate', { 'app_body' => app_body }) do
> |status, headers, body|
> +    verify(200, 'foobar', deflate_or_gzip, { 'app_body' => app_body }) do
> |status, headers, body|
>        headers.must_equal({
> -        'Content-Encoding' => 'deflate',
> +        'Content-Encoding' => 'gzip',
>          'Vary' => 'Accept-Encoding',
>          'Content-Type' => 'text/plain'
>        })
> @@ -98,15 +107,15 @@ describe Rack::Deflater do
>      app_body = Object.new
>      class << app_body; def each; yield('foo'); yield('bar'); end; end
>
> -    verify(200, app_body, 'deflate', { 'skip_body_verify' => true }) do
> |status, headers, body|
> +    verify(200, app_body, deflate_or_gzip, { 'skip_body_verify' => true
> }) do |status, headers, body|
>        headers.must_equal({
> -        'Content-Encoding' => 'deflate',
> +        'Content-Encoding' => 'gzip',
>          'Vary' => 'Accept-Encoding',
>          'Content-Type' => 'text/plain'
>        })
>
>        buf = []
> -      inflater = Zlib::Inflate.new(-Zlib::MAX_WBITS)
> +      inflater = auto_inflater
>        body.each { |part| buf << inflater.inflate(part) }
>        buf << inflater.finish
>
> @@ -118,32 +127,33 @@ describe Rack::Deflater do
>      app_body = Object.new
>      class << app_body; def each; yield('foo'); yield('bar'); end; end
>      opts = { 'skip_body_verify' => true }
> -    verify(200, app_body, 'deflate', opts) do |status, headers, body|
> +    verify(200, app_body, 'gzip', opts) do |status, headers, body|
>        headers.must_equal({
> -        'Content-Encoding' => 'deflate',
> +        'Content-Encoding' => 'gzip',
>          'Vary' => 'Accept-Encoding',
>          'Content-Type' => 'text/plain'
>        })
>
>        buf = []
> -      inflater = Zlib::Inflate.new(-Zlib::MAX_WBITS)
> +      inflater = auto_inflater
>        FakeDisconnect = Class.new(RuntimeError)
>        assert_raises(FakeDisconnect, "not Zlib::DataError not raised") do
>          body.each do |part|
> -          buf << inflater.inflate(part)
> +          tmp = inflater.inflate(part)
> +          buf << tmp if tmp.bytesize > 0
>            raise FakeDisconnect
>          end
>        end
> -      assert_raises(Zlib::BufError) { inflater.finish }
> +      inflater.finish
>        buf.must_equal(%w(foo))
>      end
>    end
>
>    # TODO: This is really just a special case of the above...
>    it 'be able to deflate String bodies' do
> -    verify(200, 'Hello world!', 'deflate') do |status, headers, body|
> +    verify(200, 'Hello world!', deflate_or_gzip) do |status, headers,
> body|
>        headers.must_equal({
> -        'Content-Encoding' => 'deflate',
> +        'Content-Encoding' => 'gzip',
>          'Vary' => 'Accept-Encoding',
>          'Content-Type' => 'text/plain'
>        })
> @@ -280,7 +290,7 @@ describe Rack::Deflater do
>          'Content-Encoding' => 'identity'
>        }
>      }
> -    verify(200, 'Hello World!', 'deflate', options)
> +    verify(200, 'Hello World!', deflate_or_gzip, options)
>    end
>
>    it "deflate if content-type matches :include" do
> @@ -334,7 +344,7 @@ describe Rack::Deflater do
>          :if => lambda { |env, status, headers, body| true }
>        }
>      }
> -    verify(200, 'Hello World!', 'deflate', options)
> +    verify(200, 'Hello World!', deflate_or_gzip, options)
>    end
>
>    it "not deflate if :if lambda evaluates to false" do
> --
> EW
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "Rack Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to rack-devel+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

-- 

--- 
You received this message because you are subscribed to the Google Groups "Rack Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to rack-devel+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: Type: text/html, Size: 10823 bytes --]