Idempotent requests retried on all error types

[assembler]

According to this: https://github.com/excon/excon/blob/master/lib/excon/middlewares/idempotent.rb#L41, idempotent requests are retried an all errors. This means that even if server responds with 404 for example, the request will be retried. It would be great if we could specify when to retry requests. 5xx sounds reasonable default.

[geemus]

When I was building it, 404 was actually one of the main things I did want to retry. It's a somewhat weird case, but we were working with eventually consistent systems, where there was sometimes a delay before something you created would appear in queries. So, I'm afraid universally changing it would likely be a breaking change.

That said, it might make sense to make it more configurable. You could probably write your own middleware to use instead of this one, for instance, as a way to specifically get what you want. And/or we could discuss what updating the existing one with options might look like.

What do you think?

[assembler]

Hey man, thanks for the response! Yes I see that universally changing the default would be a bad idea. Perhaps we can add another option that would control in which instances it is going to perform a retry. Something like this:

connection.request(:idempotent => true, :retry_on => [Excon::Error::Server, Excon::Error::Timeout, Excon::Error::Socket])

The default would be the same list of errors specified in the idempotent middleware itself. I can make a PR for this if you agree on the concept.

[geemus]

Sounds good to me. My only hesitation would be around naming the new key. I was debating between retry_on and perhaps retry_errors. The second one seems nice in being a bit more specific, but on maybe reads better? I'm kind of on the fence, what do you think? I'm in support of a PR if you'd like to go that route, we might have a little back and forth on details, but broadly this sounds like a nice enhancement. Thanks!

[assembler]

Yeah, i share your uncertainty with the name. I prefer retry_errors a bit more since it is more expressive so i have used that one in the PR but can easily change it to retry_on if you decide to go with that one. Anyway, the PR is ready and specs are passing but i'm unsure if i have structured it properly and not really sure if you test the defaults somewhere more explicitly.

[geemus]

Thanks, looks like we are on track, I just added a bit of feedback.

[stale]

This issue has been automatically marked stale due to inactivity. It will be closed if no further activity occurs. Thank you for your contributions.