ExponentialBackOff doesn't exit after `MaxElapsedTime` is passed

[slnt]

Try out the following test:

func Test_WeirdBackOffThing(t *testing.T) {
	bo := &backoff.ExponentialBackOff{
		InitialInterval:     100 * time.Millisecond,
		RandomizationFactor: 0.5,
		Multiplier:          1.5,
		MaxInterval:         time.Second,
		MaxElapsedTime:      5 * time.Second,
		Clock:               backoff.SystemClock,
	}

	err := backoff.Retry(func() error {
		return errors.New("always error")
	}, bo)

	assert.Error(t, err)
}

It will timeout. Nothing immediately seems wrong to me with the way the BackOff is configured. Why does this happen?

[slnt]

UPDATE: so it turns out the issue is not setting Stop: backoff.Stop in the backoff configuration - however this raises a question: why do I have to manually set that? Is there value to being able to use a different sentinel value than backoff.Stop?

[cenkalti]

I don't remember why it is exported in the first place. You should use NewExponentialBackOff constructor then override the fields you want.

[hamstah]

hey, I just got the same issue. The problem is that the code won't work with retry as retry looks specifically for backoff.Stop

if next = b.NextBackOff(); next == Stop {

Will not work, if you want this to work you need

		if next = b.NextBackOff(); next == b.Stop {

but this won't work without changing the interface as Backoff doesn't have a custom Stop value in it, only ExponentialBackoff.

So regardless of the reason for allowing overriding the Stop, it is currently broken as is.

[cenkalti]

@hamstah You should use the construct for creating new ExponentialBackoff instance and should not change Stop value.

[AntonioYuen]

@cenkalti sounds like a builder would be beneficial in this case. I just took this library up today and the options are pretty confusing. It's either use the default value or good luck.

I would suggest a builder starting with default values with validations on .build(). Also the builder would not allow a person to set the stop attribute. An example of a basic validation: initial interval cannot be greater than max interval.

[svkoskin]

It's either use the default value or good luck.

That isn't true, though. Nothing prevents users from modifying configurations created by constructors, and modifying structs by their users seems somewhat common in the Go ecosystem.

I used this library first time yesterday, and had a positive experience even though I needed to set a few non-default values. Even though I found it easiest to understand the customisation options through reading code.

[AntonioYuen]

@svkoskin I was able to figure out the library very quickly as well by reading the internals. But mistakes such as the one OP posted could be very common. Internals of your struct that aren't meant to be modified should be obscured from the end user, i.e. the Stop field is meant to be a default value.

Yeah modifying structs can be common but you can also set private fields by lower casing the field name, which would prevent unintended issues.

[moorow]

Not sure if anyone is still looking at this but the problem still exists.

The issue is that when NewExponentialBackOff() is called it in turn calls Reset()

// NewExponentialBackOff creates an instance of ExponentialBackOff using default values.
func NewExponentialBackOff() *ExponentialBackOff {
	b := &ExponentialBackOff{
		InitialInterval:     DefaultInitialInterval,
		RandomizationFactor: DefaultRandomizationFactor,
		Multiplier:          DefaultMultiplier,
		MaxInterval:         DefaultMaxInterval,
		MaxElapsedTime:      DefaultMaxElapsedTime,
		Stop:                Stop,
		Clock:               SystemClock,
	}
	b.Reset()
	return b
}

This sets the startTime to time.Now()

The issue with this is that it means that from the moment Retry(o Operation, b BackOff) error is invoked it effectively starts the MaxElapsedTime timer....

If 15 mins pass with no errors but on the 16th min a call fails, there will be no retry at all as it looks like the Max Elapsed Time has passed.

I would suggest something like having a sentinel value for start time when reset is called, rather than time.Now()