Unlimited retries possible in implementation, but not trough public API

[NyCodeGHG]

If I'm seeing this correctly, it should be possible to have unlimited retries in the implementation, but there seems no way of doing that via the public API, because the Default implementation defaults to 3 retries?

Is that intended or did I miss anything?

[Xuanwo]

Yes, we have not presented a direct method for that. Might you have a requirement for this?

We can use with_max_times to workaround it:

let builder = ConstantBuilder::default().with_max_times(usize::MAX);

[NyCodeGHG]

thats what i'm using right now, but feels a bit hacky, ~~also that could theoretically overflow.~~

Maybe a new function like ConstantBuilder::unlimited would be better for this?

[Xuanwo]

Maybe a new function like ConstantBuilder::unlimited would be better for this?

Seems not a good idea to me. I plan to add a FnBuilder that accept a function which will return the next delay like the following:

let f = FnBuilder::new(|| Some(Durtion::from_secs(1)));

What do you think?

[0xdeafbeef]

Maybe a new function like ConstantBuilder::unlimited would be better for this?

Seems not a good idea to me. I plan to add a FnBuilder that accept a function which will return the next delay like the following:

let f = FnBuilder::new(|| Some(Durtion::from_secs(1)));

What do you think?

It should follow the principle of least surprise. User of library shouldn't look into the implementation to find out what function will do. I've spent some time finding out why it nearly instantly exits with such setup:

backon::FibonacciBuilder::default()
            .with_jitter()
            .with_min_delay(Duration::from_millis(5))
            .with_max_delay(Duration::from_millis(100))

[Xuanwo]

It should follow the principle of least surprise. User of library shouldn't look into the implementation to find out what function will do. I've spent some time finding out why it nearly instantly exits with such setup:

Sorry for that. I will alter the jitter's API design instead to make it more clear.

[0xdeafbeef]

It should follow the principle of least surprise. User of library shouldn't look into the implementation to find out what function will do. I've spent some time finding out why it nearly instantly exits with such setup:

Sorry for that. I will alter the jitter's API design instead to make it more clear.

Oh, sorry, seems like we have misunderstanding. Jitter is not an issue, it's working as it's said in docs.

But the need to write .with_max_times(usize::MAX) is unintuitive. You expect from the builder that Builder::default() initializes with some empty values. In this case you are overwriting some predefined values.

For example, Command doesn't use ls -la as default values, it sets them as empty.

Maybe it's possible to have different build methods?

impl FibonacciBackoff {
    /// Create a new FibonacciBuilder with all fields set to None
    pub fn builder() -> FibonacciBuilder {
        FibonacciBuilder {
            jitter: None,
            min_delay: None,
            max_delay: None,
            max_times: None,
        }
    }
}

#[derive(Debug, Clone)]
pub struct FibonacciBuilder {
    jitter: Option<bool>,
    min_delay: Option<Duration>,
    max_delay: Option<Duration>,
    max_times: Option<usize>,
}

impl FibonacciBuilder {
    // ... existing methods ...

    /// Build a FibonacciBackoff with the specified maximum number of retries
    pub fn build_with_max_retries(self, max_retries: usize) -> FibonacciBackoff {
        FibonacciBackoff {
            jitter: self.jitter.unwrap_or(false),
            min_delay: self.min_delay.unwrap_or_else(|| Duration::from_secs(1)),
            max_delay: self.max_delay,
            max_times: Some(max_retries),
            previous_delay: None,
            current_delay: None,
            attempts: 0,
        }
    }

    /// Build a FibonacciBackoff with no maximum number of retries
    pub fn build_endless(self) -> FibonacciBackoff {
        FibonacciBackoff {
            jitter: self.jitter.unwrap_or(false),
            min_delay: self.min_delay.unwrap_or_else(|| Duration::from_secs(1)),
            max_delay: self.max_delay,
            max_times: None,
            previous_delay: None,
            current_delay: None,
            attempts: 0,
        }
    }
}

impl BackoffBuilder for FibonacciBuilder {
    type Backoff = FibonacciBackoff;

    fn build(&self) -> Self::Backoff {
        FibonacciBackoff {
            jitter: self.jitter.unwrap_or(false),
            min_delay: self.min_delay.unwrap_or_else(|| Duration::from_secs(1)),
            max_delay: self.max_delay,
            max_times: self.max_times,
            previous_delay: None,
            current_delay: None,
            attempts: 0,
        }
    }
}

With such api, it's impossible to miss, that it will retry 3 times and exit after that:) Also, thank you for the library!

[Xuanwo]

Thanks for the reply.

But the need to write .with_max_times(usize::MAX) is unintuitive.

It's intentional that FibonacciBackoff has the value I chose based on my own experience as a good default. I personally feel that setting usize::MAX as the default value of max_times doesn't make sense.

I will take this suggestion into account while preparing the 0.5 release.

[Xuanwo]

No actions to take so far.