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

Ruby 2.0 issue effecting all versions of Koala #346

Closed
KensoDev opened this issue Dec 17, 2013 · 19 comments
Closed

Ruby 2.0 issue effecting all versions of Koala #346

KensoDev opened this issue Dec 17, 2013 · 19 comments

Comments

@KensoDev
Copy link

Recently, I have upgraded a big application to Ruby 2.0 (all gems remained the same).

I am running version 1.4.0 of Koala (Upgraded all the way up to the RC and the problem persists).

What happens is when you run any Koala API command such as:

graph = Koala::Facebok::API.new(token)
graph.get_object 'me'

Exception is thrown

MultiJson::DecodeError: 399: unexpected token at ''

Digging through the source, the problem originates in the API class, here:

body = MultiJson.decode("[#{result.body.to_s}]")[0]

What happens is that result.body is actually a Gzipped version of the JSON response.

This is because a basic change in the Ruby 2.0 Net::HTTP class, that if you don't add a header, it will default to a header that accepts Gzip, so Facebook API will return a Gzipped response which breaks everything down the chain.

You can see the source of the Ruby code stdlib here:

https://github.com/ruby/ruby/blob/v2_0_0_353/lib/net/http/generic_request.rb#L39

The problem exists all the way down to p247 (did not check lower versions)

At a first glance, this seems like a JSON issue, or a multi_json issue, but actually nothing helps, replacing the engine or adapter in newer version doesn't help as well, you cannot fix this issue without patching HTTP.

The only possible with for it would be inside Koala to make sure that you don't pass the header.

For the meantime, if you run Ruby 2.0 and use Koala, you can patch the HTTP response to decode the JSON yourself

module DecodeHttpResponseOverride
  def initialize(h,c,m)
    super(h,c,m)
    @decode_content = true
  end

  def body
    res = super
    if self['content-length']
      self['content-length']= res.bytesize
    end
    res
  end
end

module Net
  class HTTPResponse
    prepend DecodeHttpResponseOverride
  end
end

Keep in mind this code is Ruby 1.9 incompatible, so if you want to run it in a mixed ruby env, just check for Ruby version before you prepend

if RUBY_VERSION > "1.9.3"
  prepend DecodeHttpResponseOverride
end

Again, this is NOT only a Koala bug, this breaks Twitter, Flickr and other JSON API's that respect the accept header and send back Gzip.

I will be more then happy to post a PR that fixes this, but I see that there are already a few 2.0 branches that deal with different things, so I thought I'd get this out there first.

@arsduo
Copy link
Owner

arsduo commented Dec 17, 2013

Hey Avi,

Thanks for the excellent bug report. If you want to submit a pull request, that would be great -- the PR you saw was actually for Koala 2.0, which is an intermittently active refactor I'm working on. I'd love to get Koala working cleanly in Ruby 2.0.

How does the DecodeHttpResposeOverride module work? Is @decode_content an existing flag in the class? You mentioned that this happens if we don't set a header -- would it be easier to have Koala either set a header to disable Gzip or (if the response is marked as gzip) decode the body behind the scenes before passing it to MultiJson? I'd feel more comfortable if there was a way to handle this purely in Koala and not monkeypatch a class that other libraries depend on.

What do you think?

Alex

@KensoDev
Copy link
Author

@arsduo Look here https://github.com/ruby/ruby/blob/v2_0_0_353/lib/net/http/response.rb#L83
As you can see it's just a flag that will tell the HTTPResponse class whether you want the decoding handled there or not.

I'll post a PR

@KensoDev
Copy link
Author

the PR will NOT monkey patch, it will handle the case for Koala as well, the issue was here to say what you can do PRIOR to applying the Koala patch so existing versions will still continue to work

@KensoDev
Copy link
Author

@arsduo Makes sense?

@arsduo
Copy link
Owner

arsduo commented Jan 14, 2014

Hi Avi,

Sorry about the delay (great blog post, btw). It sounds great -- a PR that handled this transparently and only for Koala requests would be perfect. If you submit it I'll make sure it gets reviewed and merged in quickly; I have a bunch of back work on issues and PRs to take care of so I could do it all at once.

Best,

Alex

@KensoDev
Copy link
Author

@arsduo No worries.

Regarding the fix, after reviewing the code a bit, I don't think it should even be in Koala, it should be in Faraday.
I am looking into maybe passing a param in make_request that will cancel that header.

I'll keep you posted with my findings.

@arsduo
Copy link
Owner

arsduo commented Feb 2, 2014

Thanks! I'll keep this open until we have something to recommend to other users being affected by it. Let me know if I can help with anything -- it's busy at work so I can't get too deeply into the problem, but still, let me know.

@tayhalla
Copy link

Are you open to a PR that addresses this issue in Koala too? This is a significant issue. Causing a lot of turmoil in my api. I'd be willing to submit.

@mislav
Copy link

mislav commented Mar 9, 2014

I find this difficult to believe. If stock Net::HTTP in Ruby 2.0 really choked on gzip responses from Twitter, Flickr, and Facebook APIs, don't you think the community would have noticed by now?

Net::HTTP#get method opts in to gzip/deflate by default since Ruby 1.9.3 if the user didn't pass an explicit accept-encoding request header. Then, if the server supports one of these encoding schemes, it sends back a compressed response and Ruby automatically decompresses it. If what you're experiencing really is caused by a Ruby bug, that would mean that Ruby is unable to decompress the response. I tried to find such a bug reported in Ruby's issue tracker, but found only an uncofirmed report: https://bugs.ruby-lang.org/issues/9467

You're saying that Ruby versions 2.0.0 p247-353 are definitely affected. I tried to reproduce your issue, but couldn't: https://gist.github.com/mislav/f1e1efcec408e847ce0c

This script uses Net::HTTP#get to hit a gzip-enabled resource. The monkeypatch added is purely for debugging purposes to detect the gzipped state before it's uncompressed. I've ran it on both Ruby 2.0.0 p247-353 and it works without problems.

@KensoDev
Copy link
Author

@mislav The problem is not with stock ruby, the problem is when you go through libraries that don't pass this header.

For example, the gem we are now discussing "koala" passes all HTTP connections through Faraday.

By default, with the dependency version, when koala calls make_request, it goes through Faraday without any header, that's exactly why it breaks koala but will not break stock ruby like you mentioned.

b.t.w, this problem exists in lots of gems (twitter, flickr, AWS). More people came across this issue as you can see here: http://tech.ftbpro.com/post/78195641092/ruby-2-1-our-experience and here: flexera-public/right_aws#167

@mislav
Copy link

mislav commented Mar 10, 2014

I cannot reproduce your problem with Koala 1.9.0, Ruby 2.0.0p353:

require 'koala'
graph = Koala::Facebook::API.new('TOKEN')
p graph.get_object("195466193802264/docs")
#=> normal, parsed response

I used a HTTP debugging proxy to inspect on the actual headers sent/received. Ruby sends this by default, which is expected:

Accept-Encoding: gzip;q=1.0,deflate;q=0.6,identity;q=0.3

and Facebook responds with a gzipped body:

Content-Encoding: gzip

The blog post which you linked to states:

Ruby 2.x Net/HTTP library asks for gzipped content by default, but does not decode it by default, which makes some JSON parsing of HTTP requests break.

which is simply not true. Your issue is caused by flexera-public/right_http_connection#20. It monkeypatched net/http and broke it as a result. The lesson of the story is that if you're reporting a bug with Ruby/Faraday/Koala/whatever, first eliminate the culprits that monkeypatch your environment. Small, isolated scripts that try to reproduce the issue help in this regard.

@KensoDev
Copy link
Author

@mislav I am using koala 1.4.0, faraday 0.8.7.
Even when upgrading koala version to the latest (before posting this bug), problem still happened.

Which version of Faraday are you using?

b.t.w, thanks for jumping in here, your input is highly appreciated

@mislav
Copy link

mislav commented Mar 10, 2014

Have you read my response until the end? Your problem is caused by a bad monkeypatch in right_http_connection. If you never let it get required in your project, you won't have a problem with gzipped responses. The fix to right_http_connection hasn't been released yet, I think.

@KensoDev
Copy link
Author

@mislav I did, sorry for not clarifying that specifically.
I am using version 3.0.0 of right_aws, and version 1.3.0 of right_http_connection.

In this version of right_http_connection there's no decode_content at all.

@mislav
Copy link

mislav commented Mar 10, 2014

That version of right_http_connection is broken as well. If I add this to my script, it starts replicating your problem:

gem 'right_http_connection', '1.3.0'
require 'right_http_connection'

@KensoDev
Copy link
Author

Likely anyone that encountered this problem had the gem included and required in this case.

@mislav
Copy link

mislav commented Mar 10, 2014

Yep, people had either this or right_aws required in their project and that activated the bad monkeypatch.

I hope that this is enough info for you to close this issue as not a problem with Koala, nor Faraday, nor Ruby.

@KensoDev
Copy link
Author

@mislav Absolutely, that's more than enough actually.
Issue closed.

@mekhovov
Copy link

this issue fixed in v 1.5.0 gem 'right_http_connection', '1.5.0'
this is PR flexera-public/right_http_connection#20

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

5 participants