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

[feature request] display warning when retry-values are set, but request is not marked idempotent #802

Open
fwolfst opened this issue Dec 7, 2022 · 3 comments
Labels

Comments

@fwolfst
Copy link
Contributor

fwolfst commented Dec 7, 2022

From what I can tell, retry_{limit,interval,errors} are only evaluated if the request is marked idempotent. As a user, this can lead to false confidence (because the request will not be retried).

I can draft a PR that uses Excon.display_warning in case these are set, but not idempotent.

@fwolfst
Copy link
Contributor Author

fwolfst commented Dec 7, 2022

(But need assurance that this is a good idea and has chances to be merged)

@geemus
Copy link
Contributor

geemus commented Dec 7, 2022

@fwolfst Thanks for reaching out. That feature request makes a lot of sense to me. There are some checks in place already for valid parameters too, so that also fits. Unfortunately, I think implementing it in a good way might be a little bit more involved than what it might seem at first glance (at least as things are currently implemented).

Idempotence is implemented via a middleware, which defaults to being utilized (though it gets skipped depending on configuration options). That said, it's possible for users to modify the middleware stack and remove the middleware if they wanted to. I'm not sure how likely it is in practice to do this, but if you knew idempotence wasn't going to be used, removing it would technically make things run slightly faster by skipping those code paths altogether.

So, the most straightforward fix might be for middlewares to somehow specify the parameters they recognized so that only the middlewares in current usage would be checked against. That said, as I think through this I wonder a bit if I should think about simplifying and streamlining some of the middleware stuff anyway (it's always been a challenge to work with).

Also, with all that said, I don't want perfect to be the enemy of the good. As an interim thing we could potentially just add some logic for the time being relating to the parameter checking that would first check to see if the idempotent middleware was in use and then check these other things. It's not as generic as the other options I described, but it's also much more approachable and simple until I have a chance to think about doing some of the more involved fixes.

Does that all make sense? What do you think about the options?

@github-actions
Copy link

github-actions bot commented Feb 6, 2023

This issue has been marked inactive and will be closed if no further activity occurs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants