ihrwein / backoff Goto Github PK

I just hit #16 and found it counter-intuitive.
Would have been nicer if it failed to compile with a nice error message, suggesting to choose between async-std or tokio.

Thanks for this useful crate!

I ran into some unexpected behavior around the timing of retries and finally got to the bottom of it. When a transient error occurs, backoff::future::Retry sets the sleeper delay but then immediately invokes the operation function, without waiting for the delay to elapse:

The future returned by the operation doesn't get polled until after the delay has elapsed, but simply invoking the operation function may be sufficient for the relevant operation (e.g., a DB query) to be initiated. In my case, I observed that a DB operation was completing prior to the backoff delay elapsing, when it shouldn't have even been initiated at that point.

Whether this causes a problem depends on the structure of the operation function. If the whole thing is wrapped in an async { ... } block, then it appears that the code doesn't get executed until the first poll, which is after the backoff delay. However, if the function executes some code and then returns a future, the operation will start immediately, in parallel with the backoff delay.

My suggestion is to defer the invocation of this.operation until after the backoff delay elapses, or at least to document this nuance :-)

I have used a Python decorator in the past, and wondered if you might consider providing a proc_macro to reduce boiler plate. I'm quit new to proc_macro, but I could give it a try.

examples/retry.rs could become:

#[retry(ExponentialBackoff)]
fn fetch_url(url: &str) -> Result<String, Error<reqwest::Error>> {
    println!("Fetching {}", url);
    let mut resp = reqwest::blocking::get(url)?;

    let mut content = String::new();
    let _ = resp.read_to_string(&mut content);
    Ok(content)
}

Hey there. I was wondering if there was any possibility of drafting a release on crates.io for this?

There are several quality of life improvements in the commit log since the release in February, and we would like to expose this functionality as well.

Best, and thanks a lot for this crate 🙏

This repo hasn't had input from its author in 17 months. No commits, no issues answered, no PRs addressed. (Not complaining, just an observation.)

Has someone already forked and tried to address the issues and PRs that have come up or is there another crate that has been serving the same purpose?

Does someone know the protocol for the Rust docs.rs site when it comes to crates that may no longer be supported?

ExponentialBackfff -> ExponentialBackoff

I might be able to make a PR for this at some point, but I've been busy, so, here's this just to document the typos

use chrono::prelude::*;

fn main() {
    let bkoffcfg = backoff::ExponentialBackoff {
        max_interval: Duration::from_secs(5),
        max_elapsed_time: Some(Duration::from_secs(15)),
        ..backoff::ExponentialBackoff::default()
    };
    let max = bkoffcfg
        .max_elapsed_time
        .map(chrono::Duration::from_std)
        .unwrap()
        .unwrap();

    let start = Local::now();
    println!("{} Starting", start.to_rfc3339());
    let _result = backoff::retry(bkoffcfg, || {
        let now = Local::now();
        println!("{} Called ({} elapsed)", now.to_rfc3339(), now - start);
        Result::<(), _>::Err(backoff::Error::Transient(()))
    });
    let finished = Local::now();
    let elapsed = finished - start;
    println!("{} Finished (took {})", finished.to_rfc3339(), elapsed);
    assert!(
        elapsed < max,
        "Max elapsed time was {}, but backoff took {}",
        max,
        elapsed
    );
}

Example output:

2021-08-11T11:29:51.579538-04:00 Starting
2021-08-11T11:29:51.580198-04:00 Called (PT0.000660S elapsed)
2021-08-11T11:29:52.271778-04:00 Called (PT0.692240S elapsed)
2021-08-11T11:29:52.805305-04:00 Called (PT1.225767S elapsed)
2021-08-11T11:29:53.951580-04:00 Called (PT2.372042S elapsed)
2021-08-11T11:29:55.591119-04:00 Called (PT4.011581S elapsed)
2021-08-11T11:29:57.967951-04:00 Called (PT6.388413S elapsed)
2021-08-11T11:30:01.500511-04:00 Called (PT9.920973S elapsed)
2021-08-11T11:30:04.147603-04:00 Called (PT12.568065S elapsed)
2021-08-11T11:30:08.863625-04:00 Called (PT17.284087S elapsed)
2021-08-11T11:30:08.863691-04:00 Finished (took PT17.284153S)
panicked at 'Max elapsed time was PT15S, but backoff took PT17.284153S'

This should never have happened, since it was already beyond max_elapsed_time:

2021-08-11T11:30:08.863625-04:00 Called (PT17.284087S elapsed)

When the previous attempt finished, retry should have computed when the next attempt would be, checked if it was beyond max_elapsed_time, and returned.

I can't get the example to work. I'm new to Rust and have a dynamic language backgroud, so I may be missing something obvious.

#[cfg(test)]
mod tests {
    #[test]
    fn exponential_backoff() {
        use backoff::{ExponentialBackoff, Operation};
        let op = || {
            if false {
                Err("bar")
            } else {
                Ok("foo")
            }
        };
        let mut backoff = ExponentialBackoff::default();
        assert_eq!("I wish I had a foo.", op.retry(&mut backoff).unwrap());
    }
}

Something's wrong with my closure, I guess.

error[E0599]: no method named `retry` found for type `[closure@src/lib.rs:422:18: 428:10]` in the current scope
   --> src/lib.rs:430:46
    |
430 |         assert_eq!("I wish I had a foo.", op.retry(&mut backoff).unwrap());
    |                                              ^^^^^
    |
    = note: op is a function, perhaps you wish to call it
    = note: the method `retry` exists but the following trait bounds were not satisfied:
            `[closure@src/lib.rs:422:18: 428:10] : backoff::Operation<_, _>`

I'm not sure how to proceed. I tried a bunch of different things, like giving my closure an explicit return:

let op = || -> Result<&str, &str> {...}

I use the crate with wasm-bindgen and async-std and it fails to compile with the following errors.

error: future cannot be sent between threads safely
   --> /Users/ander/.cargo/registry/src/github.com-1ecc6299db9ec823/backoff-0.4.0/src/future.rs:237:9
    |
237 |         Box::pin(::async_std_1::task::sleep(dur))
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future is not `Send`
    |
    = help: within `impl Future<Output = ()>`, the trait `std::marker::Send` is not implemented for `*mut u8`
note: future is not `Send` as this value is used across an await
   --> /Users/ander/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.12.0/src/io/timeout.rs:43:5
    |
40  |         timeout: timer_after(dur),
    |                  ---------------- has type `async_std::utils::timer::Timer` which is not `Send`
...
43  |     .await
    |     ^^^^^^ await occurs here, with `timer_after(dur)` maybe used later
44  | }
    | - `timer_after(dur)` is later dropped here
    = note: required for the cast from `impl Future<Output = ()>` to the object type `dyn Future<Output = ()> + std::marker::Send`

error: future cannot be sent between threads safely
   --> /Users/ander/.cargo/registry/src/github.com-1ecc6299db9ec823/backoff-0.4.0/src/future.rs:237:9
    |
237 |         Box::pin(::async_std_1::task::sleep(dur))
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future is not `Send`
    |
    = help: the trait `std::marker::Send` is not implemented for `(dyn FnMut() + 'static)`
note: future is not `Send` as this value is used across an await
   --> /Users/ander/.cargo/registry/src/github.com-1ecc6299db9ec823/async-std-1.12.0/src/io/timeout.rs:43:5
    |
40  |         timeout: timer_after(dur),
    |                  ---------------- has type `async_std::utils::timer::Timer` which is not `Send`
...
43  |     .await
    |     ^^^^^^ await occurs here, with `timer_after(dur)` maybe used later
44  | }
    | - `timer_after(dur)` is later dropped here
    = note: required for the cast from `impl Future<Output = ()>` to the object type `dyn Future<Output = ()> + std::marker::Send`

We've ran into a surprising behaviour where backoff didn't retry a failed Future even though the error was clearly mapped to backoff::Error::Transient.

After some digging, we discovered that the default value of max_elapsed_time is 15 minutes and in our case, the Future only failed several hours in. For context, our Future first establishes a websocket connection and then reads values from it in a loop. This connection can fail at any time in which case we would like to re-establish the connection (but only if it failed for certain reasons), hence our use of backoff.

I would like to suggest to either remove max_elapsed_time entirely or at least set it to None by default. It is very surprising behaviour that the total runtime of a Future influences whether or not it will actually be retried once it completes with an error.

Thoughts?

Because tokio releases version 0.3 https://crates.io/crates/tokio, please update the dependencies, thanks

If feasible and in line with the desired semantic, it would be useful to implement Clone for backoff::ExponentialBackoff.

They're in the code but cannot be found here: https://docs.rs/backoff/0.2.0/backoff/?search=future.

I'm out of time, so I cannot look into the cause but it's probably trivial.

https://github.com/ihrwein/backoff/blob/master/src/exponential.rs#L28
Here is a typo.

Is it possible to have a max retry count being set on ExponentialBackoff?

I am trying to add a retry logic to reqwest::RequestBuilder by extending it with trait.

#[async_trait]
pub trait SendRetry {
    async fn send_retry(
        self,
        retry_period: Duration,
        max_elapsed_time: Duration,
    ) -> Result<Response>;
}

#[async_trait]
impl SendRetry for reqwest::RequestBuilder {
    async fn send_retry(
        self,
        retry_period: Duration,
        max_elapsed_time: Duration,
    ) -> Result<Response> {
        
        let op = || async {
            let cloned = self
                .try_clone()
                .ok_or(backoff::Error::Permanent(anyhow::Error::msg("this request cannot be cloned")))?;

            let response = 
            cloned.send().await
            .map_err(|err| backoff::Error::Transient(anyhow::Error::from(err)))?;
            Ok(response)
        };

        let result = op
            .retry(ExponentialBackoff {
                current_interval: retry_period,
                initial_interval: retry_period,
                max_elapsed_time: Some(max_elapsed_time),
                ..ExponentialBackoff::default()
            })
            .await?;

        Ok(result)
    }
}

but i get the following exception

error[E0698]: type inside `async fn` body must be known in this context
  --> utils\src\lib.rs:47:14
   |
47 |             .retry(ExponentialBackoff {
   |              ^^^^^ cannot infer type for type parameter `E`
   |
note: the type is part of the `async fn` body because of this `await`
  --> utils\src\lib.rs:46:22
   |
46 |           let result = op
   |  ______________________^
47 | |             .retry(ExponentialBackoff {
48 | |                 current_interval: retry_period,
49 | |                 initial_interval: retry_period,
...  |
52 | |             })
53 | |             .await?;
   | |__________________^

Any idea how to fix this ?

It looks like there is no native way to check whether a strategy gave up due to reaching its end with no success (constantly hitting transient errors) and due to a permanent error.

This can be done depending on the returned error, but some variants may be used both as permanent or transient, meaning that the inner error may have to include whether it is transient or permanent. This adds a lot of boilerplate. e.g.

enum RetryError {
  // Permanent
  Y,
  Z,
  // Transient
  B, 
  C
  // Both (bool indicates whether this is permanent or transient)
  N(bool),
  M(bool),
}

TL;DR: It'd be great if retry and retry_notify could return a flag indicating whether it stopped due to an accumulation of transient errors or due to a permanent one.

I want a greater duration between the initial failure and the first retry. The docs show a lot of examples with ExponentialBackoff::default(), what's the recommended way to configure it? Is it sensible to do something like:

let backoff = ExponentialBackoff {
		initial_interval: std::time::Duration::from_secs(1),
		current_interval: std::time::Duration::from_secs(1),
		..Default::default()
	};

Does current_interval need to be set to the same value as initial_interval?

I tried to apply the async backoff example to my code, but kept running into errors, so I just tried to compile the raw example, but it fails with exactly the same errors.
This code:

use backoff::{future::FutureOperation as _, ExponentialBackoff};

async fn fetch_url(url: &str) -> Result<String, reqwest::Error> {
  (|| async {
    println!("Fetching {}", url);
    Ok(reqwest::get(url).await?.text().await?)
  })
    .retry(ExponentialBackoff::default())
    .await
}

#[tokio::main]
async fn main() {
  match fetch_url("https://www.rust-lang.org").await {
    Ok(_) => println!("Successfully fetched"),
    Err(err) => panic!("Failed to fetch: {}", err),
  }
}

causes these errors:

error[E0432]: unresolved import `backoff::future`
 --> src/main.rs:1:15
  |
1 | use backoff::{future::FutureOperation as _, ExponentialBackoff};
  |               ^^^^^^ could not find `future` in `backoff`

error[E0599]: no method named `retry` found for closure `[closure@src/main.rs:4:3: 7:5 url:_]` in the current scope
 --> src/main.rs:8:6
  |
8 |     .retry(ExponentialBackoff::default())
  |      ^^^^^ method not found in `[closure@src/main.rs:4:3: 7:5 url:_]`
  |
  = note: `(|| async {
              println!("Fetching {}", url);
              Ok(reqwest::get(url).await?.text().await?)
            })` is a function, perhaps you wish to call it

with a cargo toml dependency including backoff = ">=0.2.1"

I try to use dependency backoff = { version = "0.4.0", futures = ["wasm-bindgen"] }

error: the wasm32-unknown-unknown target is not supported by default, you may need to enable the "js" feature. For more information see: https://docs.rs/getrandom/#webassembly-support
   --> /Users/ycuk/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.2.3/src/lib.rs:219:9
    |
219 | /         compile_error!("the wasm32-unknown-unknown target is not supported by \
220 | |                         default, you may need to enable the \"js\" feature. \
221 | |                         For more information see: \
222 | |                         https://docs.rs/getrandom/#webassembly-support");
    | |_________________________________________________________________________^

error[E0433]: failed to resolve: use of undeclared crate or module `imp`
   --> /Users/ycuk/.cargo/registry/src/github.com-1ecc6299db9ec823/getrandom-0.2.3/src/lib.rs:246:5
    |
246 |     imp::getrandom_inner(dest)
    |     ^^^ use of undeclared crate or module `imp`

After clone repo and replace, it works well:

-getrandom = "0.2"
+getrandom = { version = "0.2", features = ["js"] }

The backoff::future::retry works well on wasm32-unknown-unknown with tokio as long as there is never an error that triggers the backoff. Once a backoff is triggered the TokioSleeper is used and tokio::time is used.

The problem with this is that on wasm32-unknown-unknown with no time source, a panic is caused by this attempt to sleep with tokio. This is because tokio only uses the std time which isn't implemented. The following panic will be seen:

panicked at 'time not implemented on this platform', library/std/src/sys/wasm/../unsupported/time.rs:13:9

Stack:

<?>.wasm-function[console_error_panic_hook::Error::new::hf6b67bbb1aa769d8]@[wasm code]
<?>.wasm-function[console_error_panic_hook::hook_impl::h1826ea7fdfe0e730]@[wasm code]
<?>.wasm-function[console_error_panic_hook::hook::ha2b94ff4b371fbc5]@[wasm code]
<?>.wasm-function[core::ops::function::Fn::call::h80ff1f496bb96ef1]@[wasm code]
<?>.wasm-function[std::panicking::rust_panic_with_hook::h70a0e195f4db2a29]@[wasm code]
<?>.wasm-function[std::panicking::begin_panic_handler::{{closure}}::hdcfc819ce836829e]@[wasm code]
<?>.wasm-function[std::sys_common::backtrace::__rust_end_short_backtrace::h53cabafab5b09ada]@[wasm code]
<?>.wasm-function[rust_begin_unwind]@[wasm code]
<?>.wasm-function[core::panicking::panic_fmt::h751be80779d42b53]@[wasm code]
<?>.wasm-function[std::time::Instant::now::h191947beee8509bf]@[wasm code]
<?>.wasm-function[tokio::time::instant::variant::now::h3c864f6578f780bc]@[wasm code]
<?>.wasm-function[tokio::time::instant::Instant::now::haaab15ba5b70db5b]@[wasm code]
<?>.wasm-function[tokio::time::driver::sleep::sleep::ha840de8dc1dc85fb]@[wasm code]
<?>.wasm-function[<backoff::future::TokioSleeper as backoff::future::Sleeper>::sleep::hd6eb274f5257e1c5]@[wasm code]
<?>.wasm-function[<backoff::future::Retry<S,B,N,Fn,Fut> as core::future::future::Future>::poll::h5307a8e7cbba1773]@[wasm code]

When the backoff crate has the wasm-bindgen feature enabled, it would be ideal if it leaned on another form of sleeping, such as using gloo-timer, or if there was a way to choose the time that is used. I note that this crate already uses instant instead of std Instant, so something like this would be aligned with that.

Checklist for 0.3.0:

• tokio 1.0 support #23
• Operation/FutureOperation -> FnMut, Future simplification
• Listing all breaking changes in README with migration instructions
• All dependencies are up to date
• Any planned breaking changes in API resolved
• Simplified README

Postponed for a later release:

• no_std support

I have a piece of code that essentially does something like this:

let download_fn = || async {
    Err(backoff::Error::Permanent(()))
};
let result = download_fn.retry(ExponentialBackoff::default()).await;

I have observed that this causes retry to retry multiple times even though error is clearly defined as permanent.

The definition of trait Operation<T, E> is:

fn call_op(&mut self) -> Result<T, Error<E>>;

so, the compiler cannot regard an asynchronous function as being Operation, since all asynchronous functions returns Future<...>, thus one cannot call a retry function to it.

Is there a way to perform retrying on an asynchronous function?

Some protocols return a value when a request might be served after they return a 429 Too Many Requests response. This is effectively a transient error where we know the exact time when we should retry our request.

It would be nice if we could check our response and if it contains such info override the next_backoff value with a backoff error, for example:

Error::TooManyRequests(e, Duration::from_secs(the_suggested_value))

I'm trying to use backoff::future::Retry using gloo-timers::future::TimeoutFuture as Sleeper::Sleep on WASM/Web, which is !Send. Sleeper currently requires type Sleep to be Send, but it seems this is not strictly necessary for the operation of Retry. I do understand, however, it'd technically be a visible API change to remove the Send bound, since the trait is public and not sealed, but it doesn't seem like a breaking change for the intended use of the Sleeper trait.

Hey there!

Thanks very much for authoring this crate. Would you be willing to release a new version with the version bump for rand (0.7) that's in master now? The backoff's dependency on rand:0.6 is artificially increasing the number of dependencies of my crates when some of their other dependencies use rand:0.7.

Thanks!

I'm trying to replicate your async demo in a project.

When using:
backoff = { version = "0.3.0", features = ["tokio"] }

I see this:

9 | use tokio::time::Duration;
  |     ^^^^^ use of undeclared crate or module `tokio`

in /.cargo/registry/src/github.com-1ecc6299db9ec823/backoff-0.3.0/src/future.rs

When using just:
backoff = "0.3.0"

I get:

10 | use backoff::future::retry;
   |              ^^^^^^ could not find `future` in `backoff`

When building the example from master it works and runs fine.

What the proper client setup for using the async pattern as found in the example?