Threading Bug Patches

Threading Bug Patches

[Sunny Hirai]

I'm posting patches for bugs as I find them. I'm trying to
specifically fix the file response bug at:

http://groups.google.com/group/rack-devel/browse_thread/thread/568f5045d55aa063

I haven't solved it but I have found and am finding other bugs.

In Rack::Chunked

    def call(env)
      self.dup._call(env)
    end

    def _call(env)
      status, headers, body = @app.call(env)
      headers = HeaderHash.new(headers)

      if env['HTTP_VERSION'] == 'HTTP/1.0' ||
         STATUS_WITH_NO_ENTITY_BODY.include?(status) ||
         headers['Content-Length'] ||
         headers['Transfer-Encoding']
        [status, headers, body]
      else
        dup.chunk(status, headers, body)
      end
    end

A new instance is needed because it creates an instance variable @body
later. The body would end up being the same for requests occurring at
the same time.

Sunny Hirai

Re: Threading Bug Patches

[Ryan Tomayko]

On Wed, Jan 27, 2010 at 1:26 PM, Sunny Hirai <thesunny@gmail.com> wrote:
> I'm posting patches for bugs as I find them. I'm trying to
> specifically fix the file response bug at:
>
> http://groups.google.com/group/rack-devel/browse_thread/thread/568f5045d55aa063
>
> I haven't solved it but I have found and am finding other bugs.
>
> In Rack::Chunked
>
>    def call(env)
>      self.dup._call(env)
>    end
>
>    def _call(env)
>      status, headers, body = @app.call(env)
>      headers = HeaderHash.new(headers)
>
>      if env['HTTP_VERSION'] == 'HTTP/1.0' ||
>         STATUS_WITH_NO_ENTITY_BODY.include?(status) ||
>         headers['Content-Length'] ||
>         headers['Transfer-Encoding']
>        [status, headers, body]
>      else
>        dup.chunk(status, headers, body)
>      end
>    end
>
> A new instance is needed because it creates an instance variable @body
> later. The body would end up being the same for requests occurring at
> the same time.

I don't see how. Here's Rack::Chunked in it's entirety:

    require 'rack/utils'
    module Rack
      # Middleware that applies chunked transfer encoding to response bodies
      # when the response does not include a Content-Length header.
      class Chunked
        include Rack::Utils

        def initialize(app)
          @app = app
        end

        def call(env)
          status, headers, body = @app.call(env)
          headers = HeaderHash.new(headers)

          if env['HTTP_VERSION'] == 'HTTP/1.0' ||
             STATUS_WITH_NO_ENTITY_BODY.include?(status) ||
             headers['Content-Length'] ||
             headers['Transfer-Encoding']
            [status, headers, body]
          else
            dup.chunk(status, headers, body)
          end
        end

        def chunk(status, headers, body)
          @body = body
          headers.delete('Content-Length')
          headers['Transfer-Encoding'] = 'chunked'
          [status, headers, self]
        end

        def each
          term = "\r\n"
          @body.each do |chunk|
            size = bytesize(chunk)
            next if size == 0
            yield [size.to_s(16), term, chunk, term].join
          end
          yield ["0", term, "", term].join
        end

        def close
          @body.close if @body.respond_to?(:close)
        end
      end
    end

That should be fine under multiple concurrent threads of execution.

Thanks,
Ryan