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

wrap connection and timeout exceptions #10

Merged
merged 1 commit into from
Jan 30, 2020

Conversation

binarycode
Copy link
Contributor

This PR implements exception wrapping: any underlying exceptions caused by broken connection, timeouts, etc. are wrapped in Faraday exception classes.

The list of exceptions is based on the one used in NetHttp adapter (https://github.com/lostisland/faraday/blob/master/lib/faraday/adapter/net_http.rb). I'm not sure all of the exceptions are relevant and if other ones (like for example Protocol::HTTP*::*Error or Async::HTTP::Protocol::RequestFailed or smth like that) should be included. The most common unwrapped exceptions that I've stumbled upon while using this adapter were Errno::ETIMEDOUT, Errno::ECONNREFUSED and Errno::EHOSTUNREACH.

@ioquatix
Copy link
Member

I don’t think it should be possible for Timeout::Error to occur. What errors are you seeing?

@binarycode
Copy link
Contributor Author

Yeah, you're right, looks like this exception is specific to NetHttp adapter, and should be removed.

@ioquatix
Copy link
Member

@technoweenie what do you think of this?

What is the intent of the separate Faraday exception classes?

How do users consume the timeout exception vs the connection exception vs the ssl exception? What is the use case?

Do you know Ruby has Exception#cause?

@binarycode do you think we should just catch all errors and wrap them? OpenSSL should always be defined with async-http as there is no alternative.

@binarycode
Copy link
Contributor Author

As far as I understand (from reading lostisland/faraday#718) the Faraday::TimeoutError is intended to cover both cases of open timeout and read response timeout. While this might not be completely correct, as the open timeout should probably be covered by Faraday::ConnectionFailure, it is preseved for backward compatibility.

One way the Faraday::TimeoutError exception is different from Faraday::ConnectionFailure is that the timeout one is automatically retried by default by retry middleware (https://github.com/lostisland/faraday/blob/master/lib/faraday/request/retry.rb).

I think that we should wrap the exceptions as consistent with other adapters as possible, and looks like Errno::ETIMEDOUT is wrapped into Faraday::TimeoutError by most of them.

@ioquatix
Copy link
Member

Can you rebase on master so I can merge.

@binarycode
Copy link
Contributor Author

done

@ioquatix ioquatix merged commit 04fd933 into socketry:master Jan 30, 2020
@ioquatix
Copy link
Member

Thanks!

@ioquatix ioquatix added this to the v0.7.0 milestone Feb 5, 2020
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

Successfully merging this pull request may close these issues.

2 participants