celluloid / reel

UNMAINTAINED: See celluloid/celluloid#779 - Celluloid::IO-powered web server
https://celluloid.io
MIT License
596 stars 87 forks source link

Added a way to close IO Objects encoded as a Enumerable #129

Closed Asmod4n closed 10 years ago

Asmod4n commented 10 years ago

Webmachine-Ruby Encodes IO Objects as a Enumerable. (https://github.com/seancribbs/webmachine-ruby/blob/master/lib/webmachine/decision/helpers.rb#L27 and https://github.com/seancribbs/webmachine-ruby/blob/master/lib/webmachine/streaming/io_encoder.rb#L9) This depends on https://github.com/seancribbs/webmachine-ruby/issues/145 Another possible way to handle it would be

      # Render a given response object to the network socket
      def handle_response(response)
        @socket << render_header(response)
        return response.render(@socket) if response.respond_to?(:render)

        if response.body.respond_to?(:copy_stream)
          response.body.copy_stream(@socket)
        else
          case response.body
          when String
            @socket << response.body
          when IO
            Celluloid::IO.copy_stream(response.body, @socket)
          when Enumerable
            response.body.each { |chunk| write(chunk) }
            # Webmachine-Ruby Encodes IO Objects as a Enumerable, so it needs to be closed here.
            response.body.close if response.body.respond_to?(:close)
            finish_response
          when NilClass
            # Used for streaming Transfer-Encoding chunked responses
            return
          else
            raise TypeError, "don't know how to render a #{response.body.class}"
          end
        end
      end

But Celluloid::IO doesn't do anything then.

tarcieri commented 10 years ago

This is a bit funky because the semantics are different between the response types (e.g. IO)

If we want the contract to be that Reel consistently closes the body object after it's been streamed, it should do that consistently for all response types.

Asmod4n commented 10 years ago

So adding it to the end of handle_response would be better?

tarcieri commented 10 years ago

Yes, sure

tarcieri commented 10 years ago

Looks good, thanks