8000 [HttpClient] Parameterize list of retryable methods by jderusse · Pull Request #38426 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpClient] Parameterize list of retryable methods #38426

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 20, 2020
Merged

[HttpClient] Parameterize list of retryable methods #38426

merged 1 commit into from
Oct 20, 2020
+170 −46

Conversation

jderusse
Copy link
Member
@jderusse jderusse commented Oct 6, 2020 •
edited
Loading
Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
Tickets /
License MIT
Doc PR TODO

Retrying non-idempotent methods is not always acceptable for user. This PR adds an easy way to configure this behavior.

The RetryDeciderInterface::shouldRetry() now take the exception in parameter, in order to let decider not retrying the request when the methods should never by retried.

With #38420, this code would belongs to the RetryStrategy implementation, and would return an NeverRetryDecider when method is not allowed.

@jderusse
Copy link
Member Author
jderusse commented Oct 6, 2020

I wonder if the implementation/configuration shouldn't be more complicated:

  • 423, 425, 429, 503 => always
  • 500, 502, 504, 507, 510 => idempotent only

Maybe:
ChainDecider

  • StatusCodeDecider([423, 425, 429, 503])
  • IdempotentStatusCodeDecider([500, 502, 504, 507, 510], [GET, HEAD, PUT, DELETE, TRACE, OPTIONS, CONNECT]

@jderusse jderusse force-pushed the retry-method branch 3 times, most recently from 68a37b1 to 070c1be Compare October 6, 2020 15:16
@nicolas-grekas nicolas-grekas added this to the 5.2 milestone Oct 6, 2020
@jderusse jderusse changed the base branch from master to 5.x October 6, 2020 21:47
@jderusse jderusse force-pushed the retry-method branch 2 times, most recently from 2d35c26 to e40e360 Compare October 6, 2020 21:48
@jderusse
Copy link
Member Author
jderusse 8000 commented Oct 6, 2020

failling test are related to Could not parse version constraint >=5.x: Invalid version string "5.x" And related to async-aws that should be fixed by async-aws/aws#802

@jderusse jderusse force-pushed the retry-method branch 2 times, most recently from f6270bb to 73db919 Compare October 7, 2020 10:59
Nyholm
Nyholm reviewed Oct 7, 2020
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you. I like this. I added a bunch of questions for things I dont understand.

@jderusse jderusse force-pushed the retry-method branch 5 times, most recently from 3f9fb7a to a7f7ec5 Compare October 8, 2020 11:11
@nicolas-grekas
Copy link
Member

Let's settle on #38532 before merging (and rebasing) this one.

nicolas-grekas
nicolas-grekas reviewed Oct 14, 2020
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

for exceptions, we could use the status code 0

@jderusse jderusse mentioned this pull request Oct 14, 2020
Closed
@jderusse jderusse force-pushed the retry-method branch 2 times, most recently from 2c1ee99 to 83d8d10 Compare October 16, 2020 16:53
@fabpot
Copy link
Member
fabpot commented Oct 20, 2020

Thank you @jderusse.

@fabpot fabpot mentioned this pull request Oct 20, 2020
Closed
@fabpot fabpot merged commit 185e9ea into symfony:5.x Oct 20, 2020
@jderusse jderusse deleted the retry-method branch October 25, 2020 19:49
@fabpot fabpot mentioned this pull request Oct 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Nyholm Nyholm Nyholm left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Feature HttpClient Status: Reviewed
Projects
None yet
Milestone
5.2
Development

Successfully merging this pull request may close these issues.
5 participants
@jderusse @nicolas-grekas @fabpot @Nyholm @carsonbot
0