Handle all connection timeout messages in Patron #687

stayhero · 2017-04-20T10:59:45Z

Patron is able to differentiate between a connection timeout and a normal (send or receive) timeout. The Faraday Patron adapter throws a ::Patron::TimeoutError in case of a connection timeout.

Unfortunately the Patron adapter needs to parse the original error message thrown by the curl library. Up until now the Patron adapter expected the error message to be "Connection time-out".

curl on the other hand throws another error message when a connection timeout happens:
https://github.com/curl/curl/blob/e60fe20fdf94e829ba5fce33f7a9d6c281149f7d/lib/multi.c#L1375

Connection timed out after %ld milliseconds

Therefore to properly catch all cases of a connection timeout we need to support both error messages.

stayhero · 2017-04-20T11:02:21Z

I'm not sure if the test I wrote is a good one. I test the connection timeout against Googles 8.8.8.8 DNS server because it causes a connection timeout. If you don't like it we probably could remove the test from the Patron adapter test because the case, if a ::Patron::TimeoutError was raised correctly was not tested before anyway.

This fixes #682

iMacTia · 2017-04-21T08:55:56Z

Hi @stayhero I had a look at the failing tests to see what's happening.
Your test makes perfectly sense and in fact is working fine on some ruby versions.
In others, unfortunately, it appears like we're having another different error message from CURL:

connect() timed out!

I don't have the time unfortunately to Google it and see why we're getting this, but apparently the situation is a little more complex than just 2 error messages. I would assume you need to manage at least another one

stayhero · 2017-04-21T13:17:10Z

@iMacTia Yep, I saw that. I'll look into it over the weekend. 😄

iMacTia · 2017-10-30T09:27:58Z

Hey @stayhero, did you have a chance to look into this 😃 ?

stayhero · 2017-10-30T09:48:29Z

@iMacTia Not yet. 🙈 Can you give me a week or two, I will try to solve this. :-)

iMacTia · 2017-10-30T10:00:20Z

Of course 👍! Thanks for contributing.
Please pull latest changes from master before starting, that should prevent some building issues on Travis

iMacTia · 2017-11-12T11:14:53Z

Hey @stayhero, sorry I'm just touching base to know if there's any progress on this.
I have included it into v0.14.0 which will I'd like to ship before the end of the month.
Do you think you can find some time to work on this before then?

stayhero · 2017-11-12T18:49:49Z

Yep. I'm going to work on it right now and tomorrow.

What about the test? I guess simulating a timed out connection by trying to open 8.8.8.8 on port 88 probably isn't the best way. If you'd prefer a more stable approach (e. g. setting up an extra process listening on a port but timing out), this could take some more time.

The other stuff should be easy.

stayhero · 2017-11-12T20:01:54Z

@iMacTia Tests are passing now. I added the string that caused Travis to fail and a couple of other strings I found in the cURL github repo. I'm not sure if it's ok to raise a ::ConnectionFailed exception in cases where DNS resolving does time out but I guess it's better than raising a ::TimeoutError.

I guess you could merge this pull request but I will try to improve the test as described above.

iMacTia · 2017-11-12T20:48:10Z

Great work so far 👍!
Tomorrow I'll try to have a look and double-check the different error messages that you added. Congratulations on finding so many by the way!

If in the meantime you can also get the tests improved as you suggested above, that would be even better 😃 !

iMacTia · 2017-11-13T13:55:07Z

@stayhero error messages look all good to me!
Hard to believe there are so many!

I'm not sure if it's ok to raise a ::ConnectionFailed exception in cases where DNS resolving does time out but I guess it's better than raising a ::TimeoutError.

You're totally right on the above, and I already have a ticket for that (#718).

Regarding the tests improvement, we already have a LiveServer in our test suite (https://github.com/lostisland/faraday/blob/master/test/live_server.rb) so you may want to use that one to save some time 😃

iMacTia · 2017-11-28T17:59:42Z

Hi @stayhero I'll probably release Faraday 0.14.0 this week so please let me know if you're close to improve the tests and I'll wait to merge this is 👍

stayhero · 2017-11-28T20:00:19Z

@iMacTia Not even started working on it, unfortunately. I'm not sure about this week, as I'm rather busy lately. 🙈

iMacTia · 2017-12-05T07:30:57Z

lib/faraday/adapter/patron.rb

+          "name lookup timed out",
+          "timed out before SSL",
+          "connect() timed out"
+        ].any? { |curl_message| message.include?(curl_message) }


define this array in a constant and .freeze it to improve performances

iMacTia · 2017-12-05T07:32:47Z

No worries @stayhero, I'm happy to merge this in even without the additional test, those can be added separately.
However, there's a small change that I would like to see before merging this in and I've left a review.
No idea how I missed it last time I looked at this.
If you have 5 mins to apply the change, then happy to merge, otherwise please do let me know and I'll apply the change on top of your branch

Patron is able to differentiate between a connection timeout and a normal (send or receive) timeout. The Faraday Patron adapter throws an ::Patron::TimeoutError in case of a connection timeout. Unfortunately the Patron adapter needs to parse the original error message thrown by the curl library. Up until now the Patron adapter expected the error message to be "Connection time-out". curl on the other hand throws another error message when a connection timeout happens: see https://github.com/curl/curl/blob/e60fe20fdf94e829ba5fce33f7a9d6c281149f7d/lib/multi.c#L1375 Connection timed out after %ld milliseconds Therefore to properly catch all cases of a connection timeout we need to support both error messages.

stayhero · 2017-12-05T17:29:43Z

🤔 I currently have no idea why the Travis checks are failing now. Starting the test locally the results are like this:

$ scrip/test
[...]
  1) Error:
TestConnection#test_dynamic_proxy:
Faraday::ConnectionFailed: execution expired
    /Users/chris/.rbenv/versions/2.3.3/lib/ruby/2.3.0/net/http.rb:880:in `initialize'
    /Users/chris/.rbenv/versions/2.3.3/lib/ruby/2.3.0/net/http.rb:880:in `open'
    /Users/chris/.rbenv/versions/2.3.3/lib/ruby/2.3.0/net/http.rb:880:in `block in connect'
    /Users/chris/.rbenv/versions/2.3.3/lib/ruby/2.3.0/timeout.rb:101:in `timeout'
    /Users/chris/.rbenv/versions/2.3.3/lib/ruby/2.3.0/net/http.rb:878:in `connect'
    /Users/chris/.rbenv/versions/2.3.3/lib/ruby/2.3.0/net/http.rb:863:in `do_start'
    /Users/chris/.rbenv/versions/2.3.3/lib/ruby/2.3.0/net/http.rb:852:in `start'
    /Users/chris/.rbenv/versions/2.3.3/lib/ruby/2.3.0/net/http.rb:1398:in `request'
    /Users/chris/.rbenv/versions/2.3.3/lib/ruby/2.3.0/net/http.rb:1156:in `get'
    /Users/chris/dev/ruby/gems/faraday/lib/faraday/adapter/net_http.rb:78:in `perform_request'
    /Users/chris/dev/ruby/gems/faraday/lib/faraday/adapter/net_http.rb:38:in `block in call'
    /Users/chris/dev/ruby/gems/faraday/lib/faraday/adapter/net_http.rb:85:in `with_net_http_connection'
    /Users/chris/dev/ruby/gems/faraday/lib/faraday/adapter/net_http.rb:33:in `call'
    /Users/chris/dev/ruby/gems/faraday/lib/faraday/request/url_encoded.rb:15:in `call'
    /Users/chris/dev/ruby/gems/faraday/lib/faraday/rack_builder.rb:143:in `build_response'
    /Users/chris/dev/ruby/gems/faraday/lib/faraday/connection.rb:387:in `run_request'
    /Users/chris/dev/ruby/gems/faraday/lib/faraday/connection.rb:138:in `get'
    /Users/chris/dev/ruby/gems/faraday/lib/faraday.rb:100:in `method_missing'
    test/connection_test.rb:374:in `block in test_dynamic_proxy'
    test/connection_test.rb:18:in `with_env'
    test/connection_test.rb:373:in `test_dynamic_proxy'

86 runs, 127 assertions, 0 failures, 1 errors, 0 skips
Coverage report generated for MiniTest, Unit Tests to /Users/chris/dev/ruby/gems/faraday/coverage. 1597 / 1876 LOC (85.13%) covered.

Looks like an "issue" within net_http? 🤔

iMacTia · 2017-12-05T18:26:00Z

@stayhero I noticed that in master as well, I think that's a separate issue and you haven't introduced the bug. We're performing a call to Google and that was working but apparently now it doesn't work anymore... I'll fix that separately, thanks for adding the freeze 👍

iMacTia · 2017-12-13T17:26:19Z

@stayhero the travis issue is now fixed in master 🎉🎉🎉
Can you please pull form it so I can get this merged 😄?

stayhero · 2017-12-13T18:05:50Z

@iMacTia 👏 Done, but one test failing with Ruby 2.4.0 with SSL. 😢

iMacTia · 2017-12-13T18:33:24Z

I can't believe that.... let me have a look 😭

iMacTia · 2017-12-13T18:55:19Z

A re-run made it pass 👍
Merging 🎉

iMacTia added this to the v0.14.0 milestone Nov 10, 2017

iMacTia added the bug label Nov 10, 2017

stayhero force-pushed the fix-patron-connection-timeout-handling branch from 71a522e to 4f5f978 Compare November 12, 2017 18:55

iMacTia requested changes Dec 5, 2017

View reviewed changes

stayhero added 4 commits December 5, 2017 17:11

Include more of cURL's connection timeout messages

94f514b

Add missing cURL connection time out message

0f158dd

Use freezed constant for CURL timeout messages

3b78261

stayhero force-pushed the fix-patron-connection-timeout-handling branch from 8f5203b to 3b78261 Compare December 5, 2017 16:24

iMacTia approved these changes Dec 5, 2017

View reviewed changes

Merge branch 'master' into fix-patron-connection-timeout-handling

6491ab1

iMacTia merged commit 397166e into lostisland:master Dec 13, 2017

iMacTia mentioned this pull request Mar 9, 2018

ConnectionFailed vs TimeoutError with Patron #682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle all connection timeout messages in Patron #687

Handle all connection timeout messages in Patron #687

stayhero commented Apr 20, 2017 •

edited

Loading

stayhero commented Apr 20, 2017 •

edited

Loading

iMacTia commented Apr 21, 2017

stayhero commented Apr 21, 2017

iMacTia commented Oct 30, 2017

stayhero commented Oct 30, 2017

iMacTia commented Oct 30, 2017

iMacTia commented Nov 12, 2017

stayhero commented Nov 12, 2017 •

edited

Loading

stayhero commented Nov 12, 2017

iMacTia commented Nov 12, 2017

iMacTia commented Nov 13, 2017

iMacTia commented Nov 28, 2017

stayhero commented Nov 28, 2017

iMacTia Dec 5, 2017

iMacTia commented Dec 5, 2017 •

edited

Loading

stayhero commented Dec 5, 2017 •

edited

Loading

iMacTia commented Dec 5, 2017

iMacTia commented Dec 13, 2017

stayhero commented Dec 13, 2017

iMacTia commented Dec 13, 2017

iMacTia commented Dec 13, 2017

Handle all connection timeout messages in Patron #687

Handle all connection timeout messages in Patron #687

Conversation

stayhero commented Apr 20, 2017 • edited Loading

stayhero commented Apr 20, 2017 • edited Loading

iMacTia commented Apr 21, 2017

stayhero commented Apr 21, 2017

iMacTia commented Oct 30, 2017

stayhero commented Oct 30, 2017

iMacTia commented Oct 30, 2017

iMacTia commented Nov 12, 2017

stayhero commented Nov 12, 2017 • edited Loading

stayhero commented Nov 12, 2017

iMacTia commented Nov 12, 2017

iMacTia commented Nov 13, 2017

iMacTia commented Nov 28, 2017

stayhero commented Nov 28, 2017

iMacTia Dec 5, 2017

Choose a reason for hiding this comment

iMacTia commented Dec 5, 2017 • edited Loading

stayhero commented Dec 5, 2017 • edited Loading

iMacTia commented Dec 5, 2017

iMacTia commented Dec 13, 2017

stayhero commented Dec 13, 2017

iMacTia commented Dec 13, 2017

iMacTia commented Dec 13, 2017

stayhero commented Apr 20, 2017 •

edited

Loading

stayhero commented Apr 20, 2017 •

edited

Loading

stayhero commented Nov 12, 2017 •

edited

Loading

iMacTia commented Dec 5, 2017 •

edited

Loading

stayhero commented Dec 5, 2017 •

edited

Loading