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

Faraday parsing errors even after #31 #32

Closed
alexanderdean opened this issue Mar 29, 2014 · 12 comments
Closed

Faraday parsing errors even after #31 #32

alexanderdean opened this issue Mar 29, 2014 · 12 comments

Comments

@alexanderdean
Copy link
Contributor

Even after the fixes set out in #31, I'm still getting Faraday::Error::ParsingError:

1.9.3-p545 :013 > results = Parallel.map((1000..100000), :in_threads=>30){|idx| Desk.case(idx) }
Faraday::Error::ParsingError: 757: unexpected token at 'Too Many Requests'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/json-1.8.1/lib/json/common.rb:155:in `parse'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/json-1.8.1/lib/json/common.rb:155:in `parse'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday_middleware-0.9.0/lib/faraday_middleware/response/parse_json.rb:11:in `block in <class:ParseJson>'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday_middleware-0.9.0/lib/faraday_middleware/response_middleware.rb:48:in `call'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday_middleware-0.9.0/lib/faraday_middleware/response_middleware.rb:48:in `parse'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday_middleware-0.9.0/lib/faraday_middleware/response_middleware.rb:39:in `process_response'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday_middleware-0.9.0/lib/faraday_middleware/response_middleware.rb:32:in `block in call'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday-0.8.9/lib/faraday/response.rb:63:in `on_complete'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday_middleware-0.9.0/lib/faraday_middleware/response_middleware.rb:30:in `call'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday-0.8.9/lib/faraday/response.rb:8:in `call'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday-0.8.9/lib/faraday/response.rb:8:in `call'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday-0.8.9/lib/faraday/request/url_encoded.rb:14:in `call'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday-0.8.9/lib/faraday/request/multipart.rb:13:in `call'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday_middleware-0.9.0/lib/faraday_middleware/request/oauth.rb:42:in `call'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/desk-1.0.0/lib/faraday/request/multipart_with_file.rb:16:in `call'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday-0.8.9/lib/faraday/connection.rb:253:in `run_request'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/faraday-0.8.9/lib/faraday/connection.rb:106:in `get'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/desk-1.0.0/lib/desk/request.rb:51:in `request'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/desk-1.0.0/lib/desk/request.rb:18:in `method_missing'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/desk-1.0.0/lib/desk/client/case.rb:18:in `show_case'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/desk-1.0.0/lib/desk.rb:25:in `method_missing'
    from (irb):13:in `block in irb_binding'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/parallel-1.0.0/lib/parallel.rb:389:in `call'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/parallel-1.0.0/lib/parallel.rb:389:in `call_with_index'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/parallel-1.0.0/lib/parallel.rb:229:in `block (3 levels) in work_in_threads'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/parallel-1.0.0/lib/parallel.rb:397:in `with_instrumentation'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/parallel-1.0.0/lib/parallel.rb:227:in `block (2 levels) in work_in_threads'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/parallel-1.0.0/lib/parallel.rb:221:in `loop'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/parallel-1.0.0/lib/parallel.rb:221:in `block in work_in_threads'
    from /home/vagrant/.rvm/gems/ruby-1.9.3-p545/gems/parallel-1.0.0/lib/parallel.rb:65:in `block (2 levels) in in_threads'1.9.3-p545 :014 > exit
@alexanderdean
Copy link
Contributor Author

I've done some sleuthing and I think I know what is going on. I believe:

  • For rate limiting, Desk is returning a 429 with mime type JSON but a non-JSON body: "Too Many Requests"
  • Faraday is attempting to parse the non-JSON body as JSON and throwing an error
  • Faraday is not even reaching the desk gem's custom 4xx handler

@colinc
Copy link
Collaborator

colinc commented Mar 29, 2014

@alexanderdean you are totally right and that was a known issue (that I totally forgot to go back and fix). That bug actually effects any time the Desk API has issues (so 500 errors, etc). If you're already working on fixing it then that's awesome, however, if you want some help, I have some time today that I can work on getting it fixed.

@alexanderdean
Copy link
Contributor Author

Hi @colinc - a bit of help would be amazing - my Faraday-fu is not at all strong! Very happy to test anything you write...

@colinc
Copy link
Collaborator

colinc commented Mar 29, 2014

@alexanderdean No problem. I've got a handful of commitments this morning and then I'll jump right on this.

@alexanderdean
Copy link
Contributor Author

Amazing thanks!

@colinc
Copy link
Collaborator

colinc commented Mar 29, 2014

colinc/desk@b39d9fa

This was basically a one line fix! I ripped out some of the old XML code since they're only using JSON responses now. I guess I need to go through and find any other unnecessary XML code and clean that up as well.

I'm glad I got into that code though because I think there's some improvements that can be made to optionally allow the Desk gem to handle the rate limiting itself.

In the meantime, @alexanderdean can you see if this all works for you?

Here's the test I was using to verify the changes @alexanderdean and I made.

(0..125).each do |i|
  begin
    Desk.cases
  rescue Desk::EnhanceYourCalm => e
    sleep(e.retry_after)
  end
end

@alexanderdean
Copy link
Contributor Author

Confirmed - that's working great for me, huge thanks @colinc . Here is my test:

1.9.3-p484 :012 > require "parallel"
 => true
1.9.3-p484 :013 > results = Parallel.map((1000..100000), :in_threads=>30) do |idx|
1.9.3-p484 :014 >       begin
1.9.3-p484 :015 >           Desk.case(idx)
1.9.3-p484 :016?>       rescue Desk::EnhanceYourCalm => e
1.9.3-p484 :017?>         puts "Enhance your calm for #{e.retry_after}s"
1.9.3-p484 :018?>         sleep(e.retry_after)
1.9.3-p484 :019?>       end
1.9.3-p484 :020?>   end
Enhance your calm for 22s
Enhance your calm for 22s
Enhance your calm for 22s
Enhance your calm for 22s
Enhance your calm for 22s
Enhance your calm for 22sEnhance your calm for 22s

Enhance your calm for 22s
Enhance your calm for 22s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s
Enhance your calm for 21s

How do we get all these changes merged and a new release cut?

@rguerrettaz
Copy link

+1 for the new release. Running into same issue. Thanks for all the hard work guys!

@alexanderdean
Copy link
Contributor Author

@colinc - you okay to raise a PR into this project with your fix?

@alexanderdean
Copy link
Contributor Author

@colinc any chance you can raise a PR?

@alexanderdean
Copy link
Contributor Author

Hey @chriswarren - there are some further changes in @colinc's branch - unfortunately I don't think he ever raised a PR but his fixes are needed too...

@chriswarren
Copy link
Owner

I think this is all merged in now, so I'm closing the ticket. If it's not let us know.

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

4 participants