Skip to content

Conversation

@DanGe42
Copy link
Contributor

@DanGe42 DanGe42 commented Aug 25, 2016

Make HTTP connection failures (e.g. timeouts and TLS verification failures) visible.

I initially started out by filing an issue before figuring out how I could actually resolve this. I pasted the relevant parts of that issue below.

(For what it's worth, I discovered this while updating our SDK to use more up-to-date templates. Here's the branch I'm working on if it makes things easier.)

Current behavior

To reproduce the issue of connection timeout:

  1. Turn off network access
  2. Run this API call. Wait 5-10 seconds for Typhoeus to realize that there is no network connection at all.
2.2.3 :001 > api = SquareConnect::TransactionApi.new;
2.2.3 :003 > begin; api.charge(ACCOUNTS[:US][:sandbox].access_token, ACCOUNTS[:US][:sandbox].location_id, {amount_money: {amount: 100, currency: 'USD'}, idempotency_key: SecureRandom.uuid, card_nonce: 'fake-card-nonce-declined'}); rescue => e; $ex = e; end
 => #<SquareConnect::ApiError: SquareConnect::ApiError>

I modified this section of code slightly to see how I could resolve that above issue. It looks like this:

      unless response.success?
        if response.timed_out?
          fail ApiError.new('Connection timed out')
        else
          fail ApiError.new(:code => response.code,
                            :response_headers => response.headers,
                            :response_body => response.body),
               "#{response.status_message} - #{response.body || '<No response>'}"
        end
      end

And for some reason, it will always pick the second branch and give out an empty error message.

To reproduce the second issue (TLS verification), using above modification:

  1. Start an HTTPS server with an untrusted certificate.[1]
  2. Configure SDK to point to that host
API_HOST = 'localhost:9876'

SquareConnect.configure do |conf|
  conf.host = API_HOST
end
  1. Try making any API call:
2.2.3 :003 > api = SquareConnect::TransactionApi.new;
2.2.3 :004 >   begin; api.charge(ACCOUNTS[:US][:sandbox].access_token, ACCOUNTS[:US][:sandbox].location_id, {amount_money: {amount: 100, currency: 'USD'}, idempotency_key: SecureRandom.uuid, card_nonce: 'fake-card-nonce-declined'}); rescue => e; $ex = e; end
 => #<SquareConnect::ApiError:  - >

There is no obvious indication of what went wrong.

[1]: /usr/local/opt/openssl/bin/openssl s_server -accept 9876 -cert ../server.cert -key ../server.key -WWW (using Homebrewed OpenSSL because the OS X-provided OpenSSL is old and deprecated and supports effectively no modern TLS protocols)

Modified behavior from this pull request

Disconnected from network:

2.2.3 :002 >   api = SquareConnect::TransactionApi.new;
2.2.3 :003 >   begin; api.charge(ACCOUNTS[:US][:sandbox].access_token, ACCOUNTS[:US][:sandbox].location_id, {amount_money: {amount: 100, currency: 'USD'}, idempotency_key: SecureRandom.uuid, card_nonce: 'fake-card-nonce-declined'}); rescue => e; $ex = e; end
 => #<SquareConnect::ApiError: Couldn't resolve host name>

TLS verification:

2.2.3 :001 > api = SquareConnect::TransactionApi.new;
2.2.3 :002 >   begin; api.charge(ACCOUNTS[:US][:sandbox].access_token, ACCOUNTS[:US][:sandbox].location_id, {amount_money: {amount: 100, currency: 'USD'}, idempotency_key: SecureRandom.uuid, card_nonce: 'fake-card-nonce-declined'}); rescue => e; $ex = e; end
 => #<SquareConnect::ApiError: Peer certificate cannot be authenticated with given CA certificates>
Follow-up question

I'm curious about why we chose to use Typhoeus as the underlying HTTP library. I spoke with a few Rubyist colleagues here, and here's how the Slack conversation turned out (my personal expressed opinions might not be fair because I haven't used the library much):

coworker: I’m not sure how much I would call Typhoeus another HTTP library though
coworker: given it’s basically a libcurl wrapper
me: oh, fair enough I guess
me: the interface is kinda awful, which is what motivated me to start a mini-rant about it on this channel
coworker: Typhoeus is really only useful if you want to do a lot of parallelism. Although I don’t think there’s really a good reason at this point with HTTP.rb
coworker: Typhoeus might be faster for some parts since it goes to C?

I did a quick search and it looks like the generated SDK only uses Typhoeus via Typhoeus::Request.new. It don't use any of that fancy job queuing stuff like Typhoeus::Hydra. As for why I think Typhoeus's interface is awful, it's pretty much because I had to make this fix: the library seems opinionated about not raising errors, so you have to be very explicit about handling HTTP errors or else they'll pretty much get dropped to the floor.

@wing328 wing328 added this to the v2.2.2 milestone Aug 25, 2016
fail ApiError.new('Connection timed out')
elsif response.code == 0
# Errors from libcurl will be made visible here
fail ApiError.new(response.return_message)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that the response.code is 0, what about creating the ApiError with ApiError.new(:code => 0, :message => response.return_message) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good suggestion. Updated.

The underlying HTTP library, Typhoeus, requires you to be explicit about
error handling. Unfortunately, this also means that we can't assume that
`response.success?` will be false only when the HTTP status code is not
a 200; it could also be false when the request fails (timeouts, TLS
verification issues, etc.). This commit adds explicit error handling for
these cases.
@wing328 wing328 merged commit dd1ed12 into swagger-api:master Aug 30, 2016
@wing328 wing328 changed the title Make connection failures visible with generated Ruby SDKs [Ruby] Make connection failures visible with generated Ruby SDKs Feb 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants