Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fishwife and sinatra 1.4.x mangled content #5

Closed
jillesvangurp opened this issue May 6, 2013 · 8 comments
Closed

fishwife and sinatra 1.4.x mangled content #5

jillesvangurp opened this issue May 6, 2013 · 8 comments

Comments

@jillesvangurp
Copy link

I have a simple bit of code that serves up index.html in our application.

require 'sinatra'

class StaticContentController < Sinatra::Base
get '/' do
send_file '/Users/jilles/git/localstream/www/localstream-web/foo.html'
end
end

This worked fine up to sinatra 1.3.6. After upgrading to 1.4.2, fishwife mangles the content by prepending and appending some ramdom characters. I've been manually installing and uninstalling gems with specific versions until I narrowed it down to just the sinatra gem and the way it interacts with fishwife,. Using gem list, I can see that sinatra is the only gem that is different. I've verified it is broken for 1.4.0 and 1.4.1 as well. I've tried with a simple foo.html file that contains nothing but the word hello and it returns '6 hello 0' in the browser.

to break: gem uninstall sinatra; gem install --version '1.4.2' sinatra
and to fix: gem uninstall sinatra; gem install --version '1.3.6' sinatra

So, what is wrong here? Using either mizuno or rackup, things are fine. So obviously it is a problem with fishwife. On the other hand that works fine with 1.3.6. So that makes me wonder what changed between that version and 1.4.x that breaks things here.

Any help here would be greatly appreciated since I really like the work you've been doing with Jetty 9 integration.

These are all the gems I have installed:

backports (3.3.0)
bundler (1.3.5)
eventmachine (1.0.3 java)
faraday (0.8.7)
fishwife (1.5.0 java)
hashie (2.0.4)
httpauth (0.2.0)
jsonj-integration (1.2)
jwt (0.1.8)
multi_json (1.7.3)
multipart-post (1.2.0)
oauth (0.4.7)
oauth2 (0.8.1)
omniauth (1.1.4)
omniauth-facebook (1.4.1)
omniauth-foursquare (0.0.8)
omniauth-oauth (1.0.1)
omniauth-oauth2 (1.1.1)
omniauth-openid (1.0.1)
omniauth-twitter (0.0.16)
rack (1.5.2)
rack-openid (1.3.1)
rack-protection (1.5.0)
rack-test (0.6.2)
rjack-jetty (9.0.2.0 java)
rjack-slf4j (1.7.5.0 java)
ruby-openid (2.2.3)
sinatra (1.3.6)
sinatra-docdsl (0.3.0)
tilt (1.4.0)

@dekellum
Copy link
Owner

dekellum commented May 6, 2013

Thanks for the report. I will try to reproduce and maybe narrow down to a specific new unit test. Would it be easy enough for you to also try this with sinatra 1.4.2, fishwife 1.4.0, jetty ~> 7.6.10? Also could you include the "jruby -v" line?

@dekellum
Copy link
Owner

dekellum commented May 6, 2013

I realized I have my own case of using sinatra and send_file and tested with an upgrade to sinatra 1.4.2 on that application. I don't see any random characters. Tested with these rubies:

jruby 1.6.8 (ruby-1.9.2-p312) (2012-09-18 1772b40) (Java HotSpot(TM) Server VM 1.7.0_21) [linux-i386-java]
jruby 1.7.3 (1.9.3p385) 2013-02-21 dac429b on Java HotSpot(TM) Server VM 1.8.0-ea-b84 +indy [linux-i386]

And gems:

fishwife (1.5.0-java)
rack (1.5.2)
rack-protection (1.5.0)
rack-test (0.6.2)
rjack-jetty (9.0.2.0-java)
sinatra (1.4.2)

Note that fishwife does have a different java-optimized path for send_file than Mizuno, so a specific issue is plausible.

Is it a character encoding issue you are are seeing or something else? Perhaps you could minimize the test file and send me the full output with something like curl -i?

@jillesvangurp
Copy link
Author

Hey, I'm using jruby 1.7.3 on osx and I'd be happy to help out testing of course. This issue had me puzzled quite a bit yesterday and I spent quite a bit of time narrowing it down. I'll try to create a test for this.

I think we're both using the latest gems of everything. I actually downgraded everything and then upgraded the gems one by one. Sinatra is the one that breaks it as of 1.4.0. Something changed in that release.

I don't think that it is an encoding issue. Even the string hello in a file index.html comes out mangled as '6 hello 0'. So there seems to be stuff appended and prepended. I'm seeing similar things with real html where some characters appear before the first tag and after the last one.

BTW. Github does not allow forks of forks, so I can't send you pull requests. I can email you a patch of course.

@jillesvangurp
Copy link
Author

I tried to integrate with your test suite but it seems to behave in a different way than simply running fishwife from the command line.

I have a very minimal sinatra application that fails:

config.ru

require 'testapp'

map '/' do
run StaticContentController
end

testapp

require 'sinatra'

class StaticContentController < Sinatra::Base
get '/' do
send_file '/Users/jilles/test/hello.html'
end
end

hello.html

hello

command line

$ gem uninstall sinatra; gem install --version '1.4.2' sinatra
$ fishwife &
$ curl http://localhost:9292/
6
hello

0

$ gem uninstall sinatra; gem install --version '1.3.6' sinatra
$ fishwife &
$ curl http://localhost:9292/
hello

The same controller when connected to your test seems to work as expected so something is different there. I simply copied your spec file and made it load my controller instead of yours and then used your get function to get '/'. When I run that, everything seems as it should be.

@dekellum
Copy link
Owner

dekellum commented May 7, 2013

OK, thanks for the additional details, I can now reproduce your findings using a config.ru and standalone fishwife or rackup. What you are seeing is the application of chunked encoding by Rack::Chunked which is then effectively made "literal" by Jetty.

  • sinatra 1.3.x would ignore/replace the middleware stack completely so this was never an issue. Sinatra 1.4 apparently has a fix to preserve the middleware stack including defaults from config.ru, and Rack::Server as in this case. You can see Rack::Chunked on the app stack via fishwife -d.
  • Mizuno includes a monkey-patch which disables Rack::Chunked in lib/mizuno/rack/chunked.rb. Given the ability to set proper middleware in my apps (via Sinatra 1.3 or by explicit control from not using config.ru, Rack::Server) I have been reluctant to include this hack. This report makes me reconsider that however.
  • A workaround for Fishwife 1.5.0 is to to use the none environment, i.e. fishwife -E none which will not include the Rack::Chunked middleware.

I'm investigating possible fixes.

And BTW: You can create fishwife pull requests, see the closed examples.

@jillesvangurp
Copy link
Author

Thanks, that explains. I'll give the work around a try tomorrow.

Regarding the pull requests, I end up with redirect to my mizuno fork when I try to fork your project. I could delete my mizuno fork but that would leave the open pull requests I have there in limbo. I was about to split of my fork of that when I found your project :-)

@dekellum
Copy link
Owner

dekellum commented May 8, 2013

OK please give fishwife 1.5.1 a try, where I believe I have stamped out all potential for Rack::Chunked to do harm.

Re: pull requests: FYI: You can add my fishwife repo as an additional remote to your existing repo, and submit pull request topic branches off of the the fishwife branch, ie:

git remote add fishwife git://github.com/dekellum/fishwife.git
git fetch fishwife
git checkout -t -b fishwife fishwife/fishwife
git checkout -b for-fishwife-topic

@dekellum dekellum closed this as completed May 8, 2013
@jillesvangurp
Copy link
Author

Works great now. Thanks for looking into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants