Rescue Faraday::TimeoutError #604
Conversation
stanhu
force-pushed
the
sh-rescue-faraday-server-error
branch
from
9794a88
to
317a93d
Compare
Codecov Report
@@ Coverage Diff @@
## master #604 +/- ##
=======================================
Coverage 99.48% 99.48%
=======================================
Files 13 13
Lines 386 390 +4
=======================================
+ Hits 384 388 +4
Misses 2 2
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
stanhu
force-pushed
the
sh-rescue-faraday-server-error
branch
from
317a93d
to
d2ae1fd
Compare
lib/oauth2/client.rb
Outdated
| @@ -113,7 +113,7 @@ def request(verb, url, opts = {}) | |||
| req.params.update(opts[:params]) if opts[:params] | |||
| yield(req) if block_given? | |||
| end | |||
| rescue Faraday::ConnectionFailed => e | |||
| rescue Faraday::ConnectionFailed, Faraday::TimeoutError => e | |||
There was a problem hiding this comment.
Hmm, maybe this should raise a separate exception.
There was a problem hiding this comment.
I agree. We should create a namespaced timeout error, so that people can handle them with distinct logic. If I had dug deeper into the context of the problem I would have added it already when I did the namespaced connection error. Namespaced errors, for this scenario of dependencies handling errors, was certainly the intent for 2.0.0, and therefore this is a bug.
There was a problem hiding this comment.
@stanhu will you add the new error class?
There was a problem hiding this comment.
@pboling I added a new class, but I wonder if these errors should inherit from a base class. Faraday has ServerError and ClientError.
There was a problem hiding this comment.
Oh, but actually, that would be much more disruptive. With the inheritance code that already rescues the Faraday errors will just continue to work. If we switch the base class much more code would potentially break.
Let's consider that for a 3.0.0 release, as it would be a major breaking change.
If Faraday hits a timeout, it will raise a `Faraday::TimeoutError`. Re-raise this as an `OAuth2::ConnectionError`, reusing the logic in oauth-xx#549. This came up in omniauth/omniauth-oauth2#129.
stanhu
force-pushed
the
sh-rescue-faraday-server-error
branch
from
d2ae1fd
to
a392e88
Compare
This resolves several Rubocop lint errors.
If Faraday hits a timeout, it will raise a
Faraday::TimeoutError.Re-raise this as an
OAuth2::ConnectionError, reusing the logic in#549.
This came up in omniauth/omniauth-oauth2#129.