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

Fix automatic retries of failed requests #605

Merged
merged 3 commits into from Aug 20, 2019
Merged

Conversation

Changaco
Copy link
Contributor

Unless I'm missing something, the "less than" comparison here means that the request is retried n - 1 times instead of n times, so setting max_network_retries to 1 doesn't work and setting it to 2 only retries once.

@brandur-stripe
Copy link
Contributor

@ob-stripe You're more familiar with the code here, so will leave to you. It does look like something is off — we increment the number of retries on the first request rather than waiting for an actual retry. The test looks right, but I think it's measuring retries including the original request, which is why it works.

@ob-stripe
Copy link
Contributor

Ah yeah, looks like num_retries is incremented before the first request.

I think a slightly better fix would be to keep the comparison as is and move the num_retries increment from here to here, because the first request is not a "retry" and should not be counted as such. It would also be slightly more consistent with the stripe-ruby implementation (see here).

@brandur-stripe
Copy link
Contributor

I think a slightly better fix would be to keep the comparison as is and move the num_retries increment from here to here, because the first request is not a "retry" and should not be counted as such. It would also be slightly more consistent with the stripe-ruby implementation (see here).

+1. Going off variable names, the code is pretty confusing at right now which is probably how this bug came in in the first place.

If the number of retries is set to 3, then the total number of requests should be 4: the initial one + the three retries.
This commit modifies `PycurlClient` to set the `should_retry` attribute of `APIConnectionError` exceptions, like `RequestsClient` does.
@Changaco
Copy link
Contributor Author

I think a slightly better fix would be to keep the comparison as is and move the num_retries increment

Done.

@Changaco
Copy link
Contributor Author

Ping.

Copy link
Contributor

@ob-stripe ob-stripe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay @Changaco! Thanks again for the contribution.

@ob-stripe ob-stripe merged commit 5bb1865 into stripe:master Aug 20, 2019
@ob-stripe
Copy link
Contributor

Released as 2.35.1.

@Changaco Changaco deleted the patch-1 branch August 21, 2019 06:32
Changaco added a commit to liberapay/liberapay.com that referenced this pull request Aug 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants