Increase the default Faraday retry exceptions

[ybiquitous]

Hi, I open this issue and suggest to make the default's exception retry behavior more useful.

The current exceptions, which are handled by Faraday retrying, are Octokit::ServerError. See below:

https://github.com/octokit/octokit.rb/blob/8a88e97aaaa2d5a165d566a76499ae30a9eb0bfa/lib/octokit/default.rb#L28

On the other hand, the current Faraday handles the following exceptions by default:

• Errno::ETIMEDOUT
• Timeout::Error
• Faraday::TimeoutError
• Faraday::RetriableResponse i.e. Faraday::Error. See here.

https://github.com/lostisland/faraday/blob/099dd45f63ff99bbb343eebf7504a3cf0b10bc63/lib/faraday/request/retry.rb#L26-L29

I think it would be so useful if Octokit would handle these exceptions above additionally by default!

For example:

builder.use Faraday::Request::Retry,
            exceptions: Faraday::Request::Retry::DEFAULT_EXCEPTIONS + [Octokit::ServerError]

What do you think about the suggestion?

[tarebyte]

@ybiquitous I think it's an interesting idea, though I'm pretty sure that this would be a breaking change for the v4 client.

I'll noodle on it and get back to you.

[mrpinsky]

I am also interested in this. Specifically, my interest is in adding Faraday::TimeoutError to the exceptions retried by default, which I think is reasonable behavior, given all the other magic that Octokit provides.

For context, it looks like the current behavior was introduced here by @tjoyal. There's not much context around why only Octokit::ServerError was included, and not the default list from Faraday. Can anyone shine more light on that?

[tjoyal]

There's not much context around why only Octokit::ServerError was included, and not the default list from Faraday

Looking back at the PR, it looks like this is simply my ignorance of the specifics of the Faraday library.

I'm a fan of "work out of the box" and I must have opened that past PR as I hit the problem and had to spend time back then to figure it out.

[mrpinsky]

Thanks for the insight, @tjoyal!

@tarebyte Is this still on your radar? Happy to submit a PR if it would move things along.

[DougEdey]

I submitted a PR to fix this: https://github.com/octokit/octokit.rb/pull/1546