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

Add retries to error message #159

Merged
merged 2 commits into from
Oct 16, 2024
Merged

Add retries to error message #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e3eaf2
Add retries to error message
konstin Jun 11, 2024
93fe834
Merge branch 'main' into konsti/add-retries-to-error
eopb Oct 16, 2024
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
1 change: 0 additions & 1 deletion CHANGELOG.md
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions reqwest-middleware/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Changed
- `request_middleware::Error` is now a transparent error enum and doesn't add its own context anymore.

## [0.3.3] - 2024-07-08

### Added
Expand Down
4 changes: 2 additions & 2 deletions reqwest-middleware/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ pub type Result<T> = std::result::Result<T, Error>;
#[derive(Error, Debug)]
pub enum Error {
/// There was an error running some middleware
#[error("Middleware error: {0}")]
#[error(transparent)]
Middleware(#[from] anyhow::Error),
/// Error from the underlying reqwest client
#[error("Request error: {0}")]
#[error(transparent)]
Reqwest(#[from] reqwest::Error),
}

Expand Down
3 changes: 2 additions & 1 deletion reqwest-retry/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ All notable changes to this project will be documented in this file.
The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## [Unreleased]
## [0.6.0]

## [0.6.1] - 2024-08-08

Expand All @@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

### Changed
- Upgraded `retry-policies` to `0.4.0`.
- **Breaking Change** Errors are now reported as `RetryError` that adds the number of retries to the error chain if there were any. This changes the returned error types.

## [0.5.0] - 2024-04-10

Expand Down
3 changes: 2 additions & 1 deletion reqwest-retry/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
[package]
name = "reqwest-retry"
version = "0.6.1"
version = "0.7.1"
authors = ["Rodrigo Gryzinski <[email protected]>"]
edition = "2018"
description = "Retry middleware for reqwest."
Expand All @@ -22,6 +22,7 @@ futures = "0.3.0"
http = "1.0"
reqwest = { version = "0.12.0", default-features = false }
retry-policies = "0.4"
thiserror = "1.0.61"
tracing = { version = "0.1.26", optional = true }

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
Expand Down
14 changes: 14 additions & 0 deletions reqwest-retry/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,24 @@ mod retryable;
mod retryable_strategy;

pub use retry_policies::{policies, Jitter, RetryDecision, RetryPolicy};
use thiserror::Error;

pub use middleware::RetryTransientMiddleware;
pub use retryable::Retryable;
pub use retryable_strategy::{
default_on_request_failure, default_on_request_success, DefaultRetryableStrategy,
RetryableStrategy,
};

/// Custom error type to attach the number of retries to the error message.
#[derive(Debug, Error)]
pub enum RetryError {
#[error("Request failed after {retries} retries")]
WithRetries {
retries: u32,
#[source]
err: reqwest_middleware::Error,
},
#[error(transparent)]
Error(reqwest_middleware::Error),
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion:

using an enum here is going to make it harder for a user to match on this error now, because they have to handle an extra branch that is still just a reqwest_middleware::Error

it seems completely unnecessary, why don't you just make this a struct that has a retry count and an error?
the only place where RetryError::Error was constructed, we actually do have a retry count that we could supply.

Copy link
Contributor

Choose a reason for hiding this comment

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

for example, suppose I'm a user, and i want to do further error handling and call is_body on the underlying Reqwest::Error. https://docs.rs/reqwest/latest/reqwest/struct.Error.html#method.is_body
With this enum that requires a lot more boilerplate code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need the enum so we can insert the error message about the number of retries when we did them, while not showing an irrelevant "Failed after 0 retries" otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

that doesn't seem like such a big problem. maybe you could customize Display so that it doesn't show when the number is 0? the enum is going to create boilerplate at any call-site that wants to further inspect the error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't skip the display on 0 because it ToString has to return a string even if it's an empty string, the only way i know of how to skip a step in the error trace is error(transparent).

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't skip the display on 0 because it ToString has to return a string even if it's an empty string

i think maybe you misunderstood me, you could do something like this

pub struct Error {
   err: reqwest::Error,
   retries: u32,
}

then you could implement Display like

impl Display for Error {
    fn fmt(&self, f: ...) -> ... {
       if self.retries == 0 {
          write!(f, "{}", self.err)
       } else {
          write!(f, "Failed after {} retries: {}", self.retries, self.err)
       }
    }
}

or similar

}
23 changes: 18 additions & 5 deletions reqwest-retry/src/middleware.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
use std::time::{Duration, SystemTime};

use crate::retryable_strategy::RetryableStrategy;
use crate::{retryable::Retryable, retryable_strategy::DefaultRetryableStrategy};
use crate::{retryable::Retryable, retryable_strategy::DefaultRetryableStrategy, RetryError};
use anyhow::anyhow;
use http::Extensions;
use reqwest::{Request, Response};
Expand Down Expand Up @@ -56,14 +56,14 @@
///
/// This middleware always errors when given requests with streaming bodies, before even executing
/// the request. When this happens you'll get an [`Error::Middleware`] with the message
/// 'Request object is not clonable. Are you passing a streaming body?'.

Check warning on line 59 in reqwest-retry/src/middleware.rs

View workflow job for this annotation

GitHub Actions / Spell Check with Typos 

"clonable" should be "cloneable".
///
/// Some workaround suggestions:
/// * If you can fit the data in memory, you can instead build static request bodies e.g. with
/// `Body`'s `From<String>` or `From<Bytes>` implementations.
/// * You can wrap this middleware in a custom one which skips retries for streaming requests.
/// * You can write a custom retry middleware that builds new streaming requests from the data
/// source directly, avoiding the issue of streaming requests not being clonable.

Check warning on line 66 in reqwest-retry/src/middleware.rs

View workflow job for this annotation

GitHub Actions / Spell Check with Typos 

"clonable" should be "cloneable".
pub struct RetryTransientMiddleware<
T: RetryPolicy + Send + Sync + 'static,
R: RetryableStrategy + Send + Sync + 'static = DefaultRetryableStrategy,
Expand Down Expand Up @@ -149,7 +149,7 @@
// since the byte abstraction is a shared pointer over a buffer.
let duplicate_request = req.try_clone().ok_or_else(|| {
Error::Middleware(anyhow!(
"Request object is not clonable. Are you passing a streaming body?".to_string()

Check warning on line 152 in reqwest-retry/src/middleware.rs

View workflow job for this annotation

GitHub Actions / Spell Check with Typos 

"clonable" should be "cloneable".
))
})?;

Expand All @@ -157,7 +157,7 @@

// We classify the response which will return None if not
// errors were returned.
break match self.retryable_strategy.handle(&result) {
match self.retryable_strategy.handle(&result) {
Some(Retryable::Transient) => {
// If the response failed and the error type was transient
// we can safely try to retry the request.
Expand All @@ -183,11 +183,24 @@

n_past_retries += 1;
continue;
} else {
result
}
}
Some(_) | None => result,
Some(_) | None => {}
};

// Report whether we failed with or without retries.
break if n_past_retries > 0 {
result.map_err(|err| {
Error::Middleware(
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks wrong, on this code path, this is not a middleware error, it's a request error.

a good example of a middleware error is when the middleware cannot do something it needs to do to function, like at like 146 above.

the result variable is the result of actually making a request and it failing or succeeding somehow, so these errors are request errors, not middleware errors.

Copy link
Contributor

Choose a reason for hiding this comment

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

to make this work you need to change ReqwestMiddleware::Error so that the Reqwest variant contains your retry data

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't change reqwest::Error since it's part of the reqwest crate and not of request-middleware crate, so we need to use the dynamic error variant here.

Copy link
Contributor

Choose a reason for hiding this comment

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

you can change reqwest_middleware::Error , in fact you already did in this PR. that's what i'm suggesting

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would require changing reqwest_middleware::Error so that it has a variant specific to retry errors, this would break the isolation between the general retry stack and the specific middleware for retrying.

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah i see now, that's annoying

RetryError::WithRetries {
retries: n_past_retries,
err,
}
.into(),
)
})
} else {
result.map_err(|err| Error::Middleware(RetryError::Error(err).into()))
Copy link
Contributor

Choose a reason for hiding this comment

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

this is also not a middleware error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's the most common pattern in rust to have one error type per crate (or sometimes per top level module, like serde::de::Error and serde::ser::Error). Crates higher up in the dependency tree then have an enum where most variants are wrappers around the errors of crates they depend on. In reqwest-middleware, we want to allow a dynamic middleware stack, so neither do we know the list of error type statically, nor can we declare it at compile time, so we have to use a dyn Error such as anyhow provides. The current request_middleware::Error is insufficient when adding context as we do here, it doesn't properly model that the actual reqwest::Error instance may be lower in the stack. I don't really want to change that type to just a anyhow::Error wrapper with a method that finds you a reqwest::Error in the chain, if any, to avoid too much churn.

Copy link
Contributor

@cbeck88 cbeck88 Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the change you are making here, where normal reqwest::Error from the crate, is classified as reqwest_middleware::Error::middleware instead of reqwest_middleware::Error::request, is going to break every single existing user of this crate who attempts to actually inspect and handle these errors. because today, all those users match on reqwest_middleware::Error and 99.9% of the time it's a request error that they can further inspect.

Copy link
Contributor

Choose a reason for hiding this comment

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

it almost completely defeats the point of having request_middleware::Error::reqwest, which after this change will serve to only confuse users

Copy link
Contributor

Choose a reason for hiding this comment

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

imo, these changelog entries are not sufficient, you should indicate a semver breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current interpretation of the error enum assumes two cases:

  • There was an internal error in the middleware
  • There was a reqwest::Error we forward
    But there exists a third class of errors, those where we do have a reqwest::Error, but also additional context or a middle error with a reqwest::Error behind it. The current enum represent this kind of errors, and there exists no simple rust pattern for matching on them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, i've bumped reqwest-retry to 0.6.0, let's see what the maintainers say about reqwest-middleware.

};
}
}
Expand Down
Loading