rails streamed pages are doubly chunked encoded after going through tomcat

tolsen commented 12 years ago

Hello,

It appears that pages that are HTTP streamed using render(stream: true) in Rails 3.2.8 are being doubly chunked encoded when served by Tomcat 6.0.24 / jruby-rack 1.1.9:

=======================
HTTP/1.1 200 OK
Server: Apache-Coyote/1.1
Cache-Control: no-cache
Transfer-Encoding: chunked
X-UA-Compatible: IE=Edge,chrome=1
Set-Cookie: _jux_session=BAh7B0kiD3Nlc3Npb25faWQGOgZFRkkiJTNjYzlhOWU3M2ZmZjYzMTI5NDgzNGVhM2E5ODM2MGQyBjsAVCIQX2NzcmZfdG9rZW5JIjFLTGJlS3F0Tk53NWR1TDA3K1FtL0RZazhkL0ErVkJCTmlRNk9sSnZQK1A0PQY7AEY%3D--b8360fdbf6ce936ce74cee2a68736f085de2b4ce; domain=gowanus.jux.nu; path=/; HttpOnly
X-Request-Id: 8d8f26443467ff33fb4f2834c4af85ed
X-Runtime: 0.503000
Date: Tue, 21 Aug 2012 02:07:17 GMT
X-Rack-Cache: pass
Content-Type: text/html;charset=utf-8
Transfer-Encoding: chunked

10
<!DOCTYPE html>

c
<html class=
39
"theme-dusk userNotSignedIn  notGalleryOwner monoGallery"
30
 style="background-color:#2e2d2d">
  <head>

1

c
    <title>

8

18
=======================

Note the chunk markers and the double occurrence of the Transfer-Encoding header. Is there a way to have Tomcat dechunk the output from Rails/jruby-rack or have Rails/jruby-rack not chunk its output (if that even makes sense in the case of streaming)?

kares commented 12 years ago

Hi Tim, thanks for the report ... just for the record did you see the same issue with jruby-rack 1.1.7 ?

tolsen commented 12 years ago

Yes. I actually got the error first with 1.1.7 and then tried 1.1.9 before reporting this bug.

tolsen commented 12 years ago

It appears that someone else had a similar issue back in March:

http://netrubyonrails.blogspot.com/2012/03/rails-re-http-streaming-with-ruby-on_02.html

Note that last sentence: "In addition to the content, It spits out some random numbers and letters as it streams, but I guess I'll find the answer for that in due time too :)"

tolsen commented 12 years ago

It appears that the sinatra project gave up on getting their streaming tests to pass with trinidad (which uses tomcat and jruby-rack) about 5 months ago:

https://github.com/kares/sinatra/commit/f65520347b72edfae6c8485726720cc423a312dd

https://github.com/sinatra/sinatra/pull/482

tolsen commented 12 years ago

It looks like jruby-rack should not be sending a chunked-encoded stream to Tomcat:

http://stackoverflow.com/questions/7030885/why-is-chunked-transfer-encoding-not-being-respected-by-firefox

Unfortunately, Rails has it hardcoded to chunk-encode for streaming. I'll try monkey patching rails to not do so.

kares commented 12 years ago

actually the sinatra commits are not really related, they're doing their own streaming thing (as far as I recall it depended on EM or did the ticks in a way that assumed a single-threaded mode). unfortunately there's no rack standard for doing async responses which is kind of what rails streaming could built this upon (for now it's done self sufficiently with fibers). hopefully I'll need to manage some free time to get this done - as it seems quite a challenge with the way things are now ...

tolsen commented 12 years ago

I managed to get it working by monkey-patching Rails to not chunk encode streamed output and modifying jruby-rack to flush when Content-Length is not set.

My Rails monkey patch:


module ActionController::Streaming

  # monkey patch Rails to not stream with chunked-encoding to avoid
  # double-chunked encoding responses from Tomcat

  def _process_options(options) #:nodoc:
    super
    if options[:stream]
      if env["HTTP_VERSION"] == "HTTP/1.0"
        options.delete(:stream)
      else
        headers["Cache-Control"] ||= "no-cache"
        headers.delete("Content-Length")
      end
    end
  end

  def _render_template(options)
    if options.delete(:stream)
      view_renderer.render_body(view_context, options)
    else
      super
    end
  end

end

(The original Rails code is at https://github.com/rails/rails/blob/d1b9cf2d48b4e58da2da563107dd2783e326e287/actionpack/lib/action_controller/metal/streaming.rb#L199 )

My jruby-rack modifications: https://github.com/tolsen/jruby-rack/compare/9ded4a82f3...406d944e3b

I see two paths to implementing this:

We persuade the Rails team to offer a configuration option where chunked encoding can be turned off during streaming. My changes or similar changes are made to jruby-rack.

or
jruby-rack dechunks the chunked output from Rails (as Passenger currently does)

AFAICT, this does not require anything async. jruby-rack can block while calling each on the body.

kares commented 12 years ago

true it works backward compatibly - you can even set a "HTTP_VERSION" servlet request attribute to1.0 and it should have worked (without changing any additional code) ... as for the async I was trying to explain things as the comments seemed a bit confusing, having an async rack standard would be great since than streaming could be done async-ly (even if only using fibers) and we would be able to provide a implementation based on the servlet async context. not sure how it would impact performance but it won't block the worker thread at least.

back to the chunked issue, removing the header from Streaming seems really like something's wrong on our side (or with Tomcat) as for your jruby-rack changes they seem fine to me but we need to see how it works with other containers.

kares commented 12 years ago

so although I could not find this mentioned in the servlet spec it seems standard behaviour ... Jetty since 6.1x does this as well as Tomcat 6/7 (and thus JBoss), seems that WebSphere is doing it as well if the buffer size can not hold the written body content. seems to me that most containers would turn chunking on with code such as (unless Content-Length set) :

// flush headers 
response.flushBuffer() 
// then flush body 
while (havedata) { 
  response.getOutputStream().println(some data) 
  response.flushBuffer() 
}

thus you're right there's the 2 paths you mentioned :

I do not like this since others Rack applications can still use Rack::Chunked::Body
would solve the issue for good but sounds like yet another redundancy

the best thing would be if Rack was taking "complete" care of this and thus we patched Rack (while allowing Rack::Chunked::Body to work as expected when instantiated).

I think the currently achievable solution is similar to what you did including a ActionController::Streamingpatch. Before doing this, I'd like to look into how Rails 4 is looking (including live streaming) thus we know what to expect ... next.

Anyway thanks again for the report and the ideas to resolve this !

tolsen commented 12 years ago

I agree that solution 1 is not as realistic as we would need all of the Rack applications to offer a way to not chunk encode. But there may be are two other reasons for them not to chunk encode: rack middleware compatibility and request spec compatibility. I've run into a few rack middleware that are not compatible with streaming because they do not understand chunk encoding. And as for request specs, I had to disable some of our assertions in our request specs because of the chunk markers in the resulting bodies. I was able to reenable these assertions after monkey-patching Rails to not chunk encode.

It seems to me that there should be a rack middleware that does chunk encoding and it can be put at the end of the rack stack when it is needed. Having Rack applications chunk encode at their core just makes it that much harder for rack middleware, request specs / integration tests, and app servers to interface with them.

Also keep in mind the current state of HTTP streaming with jruby-rack and tomcat: broken for responses over a certain size. I don't see how this can be working even for Sinatra. Even if there were only partial support for those Rack applications that offered a way to not chunk encode, it would be better than what we have now.

On the other hand, dechunking (solution 2) is probably not too much code and would get full compatibility now.

kares commented 12 years ago

agree, Rails seems to be producing "non-compatible" middleware - it's very unfortunate maybe this is what you get with ODD :) on the other side Rack seems to be progressing rather slowly lately - that might be a motivator as well. would love to know whether it's planned to be addressed in Rails ... would be great if someone would look into it.

and you're right it's a shame HTTP streaming is broken with jruby-rack, seems that not a lot of our users are running 3.2 or using streaming with 3.2 or there ain't a lot of JRuby::Rack users at all :) ... I personally rarely find time to test out new Rails features, thus I pretty much rely on reports such as yours. expect this to be fixed and released next week !

tolsen commented 12 years ago

I've started a discussion on chunking on the Ruby on Rails core list. Please feel free to join in!

kares commented 12 years ago

Hey Tim, a fix for this is present on master as promised, I've implemented de-chunking just like you suggested. Some real world testing would be nice if you manage to find some time, here's a snapshot gem build: https://github.com/downloads/kares/jruby-rack/jruby-rack-1.1.10.dev.gem

I'd like to add a configuration option for cases when (for whatever reason) people'd like to turn dechunking off and maybe an initializer that (by default) removes the Rack::Chunked middleware if it's present, just in case Rails decides to do this the "intended" way ...

tolsen commented 12 years ago

Hey Kares. How do I use your gem with trinidad and bundler? I am not sure how to tell bundler to use a gem package.

kares commented 12 years ago

first gem install jruby-rack-1.1.10.dev.gem than feel free to put it into the Gemfile like any other dependency :

gem 'jruby-rack', '1.1.10.dev', :platform => :jruby
gem 'trinidad', :require => nil, :platform => :jruby

and bundle install (I usually put server dependencies into a separate group but that ain't necessary) finally run trinidad ...

tolsen commented 12 years ago

It works! Thanks! Btw, any idea how to install your gem if I use 'bundle install --deployment' ?

tolsen commented 12 years ago

Actually, I'm ok without "bundle install --deployment". I was using it on a server for performance testing but I can just use gemsets for now until 1.1.10 is released.

dukejones commented 12 years ago

Rack::Stream looks promising. Looks to be still early in its lifecycle, though.

https://github.com/intridea/rack-stream

jruby / jruby-rack

rails streamed pages are doubly chunked encoded after going through tomcat #117