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

Retry on 5xx, error on 4xx #465

Merged
merged 4 commits into from
Nov 1, 2021
Merged

Retry on 5xx, error on 4xx #465

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bac6582
Retry on 5xx, error on 4xx
rylev Oct 29, 2021
3d69a37
Onlyy retry on certain error statuses
rylev Oct 29, 2021
fe48dc6
Only validate responses in the retry policy
rylev Oct 29, 2021
8796038
Fix inverted logic
rylev Oct 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 24 additions & 10 deletions sdk/core/src/policies/retry_policies/retry_policy.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ use crate::policies::{Policy, PolicyResult, Request, Response};
use crate::sleep::sleep;
use crate::PipelineContext;
use chrono::{DateTime, Local};
use http::StatusCode;
use std::sync::Arc;
use std::time::Duration;

Expand All @@ -26,19 +27,32 @@ where
let mut retry_count = 0;

loop {
match next[0].send(ctx, request, &next[1..]).await {
Ok(response) => return Ok(response),
Err(error) => {
log::error!("Error occurred when making request: {}", error);
if self.is_expired(&mut first_retry_time, retry_count) {
return Err(error);
} else {
retry_count += 1;

sleep(self.sleep_duration(retry_count)).await;
let error = match next[0].send(ctx, request, &next[1..]).await {
Ok(response) => {
let status = response.status();
if status.as_u16() < 400 {
// Successful status code
return Ok(response);
} else if status.as_u16() < 500 {
// Server returned a client caused error
rylev marked this conversation as resolved.
Show resolved Hide resolved
return Ok(response.validate(StatusCode::OK).await?);
rylev marked this conversation as resolved.
Show resolved Hide resolved
}
// Server returned an internal error, try again
log::error!("server returned error 500 status: {}", status);
Box::new(response.validate(StatusCode::OK).await.unwrap_err())
}
Err(error) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The C# SDK retries IOExceptions so we should retry here too (assuming we can discriminate the error to errors raised by the http policy).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you saying we should only retry IO related errors? Right now any error is retried.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, sorry, that was what I meant... 😔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I can tackle that in another PR. This might require that we change the pipeline to not work with Box<dyn Error> but with azure_core::Error instead.

log::error!("error occurred when making request: {}", error);
error
}
};

if self.is_expired(&mut first_retry_time, retry_count) {
return Err(error);
}
retry_count += 1;

sleep(self.sleep_duration(retry_count)).await;
}
}
}
10 changes: 10 additions & 0 deletions sdk/core/src/response.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,16 @@ pub struct Response {
body: PinnedStream,
}

impl std::fmt::Debug for Response {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
f.debug_struct("Response")
.field("status", &self.status)
.field("headers", &self.headers)
.field("body", &"<BODY>")
.finish()
}
}

impl Response {
pub(crate) fn new(status: StatusCode, headers: HeaderMap, body: PinnedStream) -> Self {
Self {
Expand Down