Rescue Faraday timeout errors #129
Conversation
In the event of a `Net::ReadTimeout`, Faraday raises
`Faraday::TimeoutError`, and for `Net::OpenTimeout it raises
`Faraday::ConnectionFailed`. These errors may be encountered
when specifying a shorter timeout for Faraday via
`client_options: { connection_opts: { request: { open_timeout: 30, timeout: 30 } } }`
and if the request takes longer than those timeout values in seconds.
|
@tmilewski @supernova32 Would you mind reviewing, please? Thanks 😄 |
|
My main concern, and why I have not yet merged this, is that this library nor omniauth directly list Faraday as a dependency. Explicitly rescuing an error for an http library that may or may not be used by consumers of this gem seems like a smell to me. |
|
@BobbyMcWho I agree with you. If the error being rescued is not from a dependency used by the gem, then it should be rescued by the application using it, and not us. |
|
@dblessing if you need to rescue those Faraday errors, you can create a custom strategy that inherits from this or the Google one, and override that method using the same general code with your Faraday errors added |
|
@BobbyMcWho I see what you're saying. Faraday is a dependency of the The ideal case would probably be for |
|
Thanks @dblessing, I've looked through the OAuth2 code and it does appear that it has a hard dependency on Faraday. @supernova32 what do you think in that case, should we accept this PR and handle the Faraday error of our direct oauth2 dependency? Or ask that OAuth2 catch the error and re-namespace it? |
|
@BobbyMcWho 🤔 that's a tough question... We would still be rescuing from an error in a dependency of a dependency. I'm inclined to think the error should be handled in OAuth2, mainly because I'm of the mind that errors should be caught as early as possible, and not bubble up through the stack. Here is where the Faraday connection object is used on the oauth2 gem: https://github.com/oauth-xx/oauth2/blob/master/lib/oauth2/client.rb#L99 Maybe somewhere within that file is where the rescue should be added? |
|
Yeah I think I agree with that logic. Have OAuth2 bubble up a namespaced timeout or connection error and we can handle that. Could be a breaking change in their lib, however |
|
I'll open an issue and/or PR with that project and see what they say. Thanks. Let's close for now. Edit: Found an existing issue at https://github.com/oauth-xx/oauth2/issues/152. Perfect. |
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.
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.
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.
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.
* Rescue Faraday::TimeoutError 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. * Break out OAuth2::Client#request This resolves several Rubocop lint errors.
* This fix was originally proposed with a different solution via this PR: omniauth#129 * The conclusion there was that we should not rescue Faraday errors because Faraday is not a direct dependency of OAuth2. * Instead, best to rescue Faraday errors on OAuth2, then rescue OAuth2 errors here * Those changes to OAuth2 were made here: https://gitlab.com/oauth-xx/oauth2/-/merge_requests/604 * And released as part of OAuth2 gem version 2.0.2: https://gitlab.com/oauth-xx/oauth2/-/blob/866dc365bfe0d64f6cc56295a38ccd5b24143836/CHANGELOG.md#L74
In the event of a
Net::ReadTimeout, Faraday raisesFaraday::TimeoutError, and forNet::OpenTimeoutit raisesFaraday::ConnectionFailed. These errors may be encounteredwhen specifying a timeout for Faraday via
client_options: { connection_opts: { request: { open_timeout: 30, timeout: 30 } } }and if the request takes longer than those timeout values in seconds.
We are setting these timeouts for omniauth-google-oauth2 as we're sometimes seeing the OAuth requests take longer than 60 seconds. Not sure why Google is taking so long to respond, but in any case, setting a sane timeout is helpful in these cases so users don't need to wait. But if an error is raised we can't rescue it within our app and need it to be handled as a proper omniauth failure.
If you want me to write some tests, happy to take a look. I tried to find some existing examples or simple ways to test and it didn't seem all that useful - it would be stubbing Faraday or something within this gem. But happy to take a look again if you would like.