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.

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

Fix retry logic #1762

Merged
merged 3 commits into from
Dec 18, 2017
Merged

Fix retry logic #1762

merged 3 commits into from
Dec 18, 2017

Conversation

kevmoo
Copy link
Member

@kevmoo kevmoo commented Dec 16, 2017

Closes #1556

@kevmoo kevmoo requested review from nex3 and munificent December 16, 2017 05:59
@kevmoo
Copy link
Member Author

kevmoo commented Dec 16, 2017

There was a bit of an oversight in 932e76e

The innermost client – _PubHttpClient – throws on bad status codes. These exceptions blow right past the retry logic.

My solution – move the check & throw logic into a _ThrowingClient that sits outside of of the retry logic.

@kevmoo
Copy link
Member Author

kevmoo commented Dec 16, 2017

...at least one test would be a good idea...

@kevmoo
Copy link
Member Author

kevmoo commented Dec 16, 2017

Feel free to squash/merge and delete the branch if approve...

nex3
nex3 suggested changes Dec 18, 2017
View reviewed changes
Copy link
Member

@nex3 nex3 left a comment

Choose a reason for hiding this comment

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

I'm always a bit reticent to add tests for behavior that involves explicit delays, but if you want to add a test for retrying a single time I wouldn't be opposed.

lib/src/http.dart Outdated
@@ -216,10 +184,54 @@ final _pubClient = new _PubHttpClient();
/// we're waiting for them to come back up.
final _retriedHosts = new Set<String>();

class _ThrowingClient extends http.BaseClient {
Copy link
Member

Choose a reason for hiding this comment

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

Please document this.

Copy link
Member Author

Choose a reason for hiding this comment

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

PTAL

@nex3
Copy link
Member

nex3 commented Dec 18, 2017

I'll merge once Travis comes back successful.

@kevmoo
Copy link
Member Author

kevmoo commented Dec 18, 2017 via email

@kevmoo kevmoo merged commit 667281e into master Dec 18, 2017
@kevmoo kevmoo deleted the i1556_retry_retry branch December 18, 2017 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nex3 nex3 nex3 approved these changes

@munificent munificent Awaiting requested review from munificent

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kevmoo @nex3 @googlebot