cenkalti / backoff Goto Github PK

I have a function that retrieves X amounts of items from an api.

i'm using backoff with success to adjust for some networking issues,
but sometimes the backend is struggling on requests for X items, whereas X/2 might work fine, and waiting won't help.

so it would be nice if there was a way I could adjust my function based on error returns, and maybe also on successes, so that i can temporarily decrease and then increase again my value of X.

What makes this type not thread-safe?

https://github.com/cenkalti/backoff/blob/master/exponential.go#L52

Only thing I can see is a possible issue here https://github.com/cenkalti/backoff/blob/master/exponential.go#L103.

Consider the following test program:

package main

import (
    "log"
    "net"

    "github.com/cenkalti/backoff"
)

func main() {
    b := backoff.NewExponentialBackOff()
    ticker := backoff.NewTicker(b)
    for _ = range ticker.C {
        log.Println("dialing...")
        conn, err := net.Dial("tcp", "localhost:6060")
        if err != nil {
            log.Print(err)
            continue
        }
        log.Println("conn established")
        err = writePings(conn)
        log.Println("session terminated:", err)
        b.Reset()
    }
    log.Fatalf("maximum retries exceeded, exiting")
}

func writePings(conn net.Conn) error {
    for {
        _, err := conn.Write([]byte("ping\n"))
        if err != nil {
            return err
        }
        time.Sleep(5*time.Second)
    }
}

$ go run test_case.go
2014/09/04 15:58:25 dialing...
2014/09/04 15:58:25 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:58:25 dialing...
2014/09/04 15:58:25 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:58:26 dialing...
2014/09/04 15:58:26 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:58:27 dialing...
2014/09/04 15:58:27 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:58:29 dialing...
2014/09/04 15:58:29 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:58:31 dialing...
2014/09/04 15:58:31 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:58:36 dialing...
2014/09/04 15:58:36 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:58:39 dialing...
2014/09/04 15:58:39 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:58:45 dialing...
2014/09/04 15:58:45 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:58:52 dialing...
2014/09/04 15:58:52 conn established
2014/09/04 15:59:02 session terminated: write tcp 127.0.0.1:6060: broken pipe
2014/09/04 15:59:08 dialing...
2014/09/04 15:59:08 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:59:08 dialing...
2014/09/04 15:59:08 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:59:09 dialing...
2014/09/04 15:59:09 dial tcp 127.0.0.1:6060: connection refused
2014/09/04 15:59:10 dialing...
2014/09/04 15:59:10 dial tcp 127.0.0.1:6060: connection refused

Specifically, the following two lines:

2014/09/04 15:59:02 session terminated: write tcp 127.0.0.1:6060: broken pipe
2014/09/04 15:59:08 dialing...

After the conn was terminated, I call b.Reset(), so I'd have expected the ticker to fire again immediately. Unfortunately, it appears that it is already timed to fire after the previous backoff time, and waits several seconds before its first attempt to reconnect. After that, it behaves as I'd expect given the reset.

Today when backoff is imported in a go.mod enabled module this happens to mymod go.mod (note the +incompatible).

Add a go.mod that makes backoff compatible with the GO module specification.

Thanks.

module mymod

go 1.12

require (
	github.com/cenkalti/backoff v2.1.1+incompatible
)

Although there is a go.mod file in the repository, the latest version that can be used with Go modules (unless we force it to use a commit) is v2.1.1. This is because the tag is v2.2.0, but the import path is still github.com/cenkalti/backoff.

With go modules, once you get to v2 and up, you need to update the import path to github.com/cenkalti/backoff/v2 and so on.

backoff.go gives EOF error when used from go get

I believe only block comments /* */ are allowed before the package declaration and single line comments // anywhere else

Running this test function produces very different results depending on whether the Reset() call is there or not. Without the reset, the test run takes about 40seconds, and with it it's ~4.5seconds. Surely that is a bug?

func TestBackoff(t *testing.T) {
	ebo := backoff.NewExponentialBackOff()
	ebo.MaxElapsedTime = 30 * time.Second
	ebo.MaxInterval = 20 * time.Second
	ebo.InitialInterval = 50 * time.Millisecond

	//ebo.Reset()
	start := time.Now()
	for i := 0; i < 10; i++ {
		next := ebo.NextBackOff()
		time.Sleep(next)
	}
	fmt.Println(time.Since(start))
}

And because backoff.Retry() calls .Reset automatically, the result of using backoff manually (using .NextBackOff) vs calling .Retry is very different.

looking at this line, I think there can possibly be a case where the minimum interval can be negative, if delta < minInterval.

I tried solving it by changing that line to:

import "math"
var minInterval = math.Min(0, float64(currentInterval)-delta)

but then the tests fail.

wdyt?

just a simple spelling error / typo. Pull request already out there

I get this error:
go list -m: github.com/cenkalti/[email protected]+incompatible: invalid version: +incompatible suffix not allowed: module contains a go.mod file, so semantic import versioning is required

I think for versions > 1.0 you need to have the code in a sub-dir. In any case, go 1.13 does not like how you are doing versioning and giving errors.

I was browsing through the code, and noticed the runtime.SetFinalizer(ticker, stop) call at https://github.com/cenkalti/backoff/blob/master/ticker.go#L37

I suspect it doesn't work. As far as I understand it, goroutines are "gc roots" like in practically every other language, so as long as the run goroutine is running there is always something referencing the ticker, so it never gets garbage collected.

A quick test fails tor produce any "finalizer ran" results:

import (
	"testing"
	"time"
	"runtime"
)

type ticker struct{}
func (t *ticker) run() {
	// run forever and just keep sleeping
	for {
		<- time.After(10 * time.Millisecond)
	}
}

func runner(t *testing.T) {
	thing := ticker{}
	go thing.run()
	runtime.SetFinalizer(&thing, func(_ *ticker){ t.Log("Finalizer ran")})
}

func TestGc(t *testing.T) {
	runner(t)
	t.Log("Before sleep")
	<-time.After(100 * time.Millisecond)
	t.Log("Before gc")
	runtime.GC()
	t.Log("After gc")
	<-time.After(100 * time.Millisecond)
	t.Log("After sleep")
	<-time.After(1000 * time.Millisecond)
	t.Fail()
}

... though I completely realize this kind of thing is difficult to test conclusively.

Am I missing something / is there evidence that this works? I assume the cost is minimal, but if I'm right I'd rather not leave it in place where it might mislead readers.

This package works great, nice work!
However, it took me quite a while to figure out how to perform advanced operations. For example, how to execute an HTTP GET request using backoff. This was because documentation is a bit lacking, and required me to read the source in order to understand how to use it. Improving the docs could greatly shorten the learning curve.

Things that could be improved:

• Readme is too short
• Godoc could be more clearer
• Be gofmt, go vet, go lint compliant
• Example files (example_test.go)
• Advanced examples
• Type alias the operation and notify, so it would be easier to understand how this package works:

type Operation func() error
type Notify func(error, time.Duration)

func Retry(Operation, BackOff) error
func RetryNotify(Operation, BackOff, Notify)

Passing an exponential backoff to a ticker and then calling "NextBackOff" on the backoff strategy from inside the ticker loop creates data race between the ticker calling the the method and the client code calling the method.

WARNING: DATA RACE
Read at 0x00c420099508 by main goroutine:
  math/rand.(*rngSource).Int63()
      /usr/local/Cellar/go/1.9/libexec/src/math/rand/rng.go:241 +0x9a
  math/rand.(*Rand).Int63()
      /usr/local/Cellar/go/1.9/libexec/src/math/rand/rand.go:82 +0x51
  math/rand.(*Rand).Float64()
      /usr/local/Cellar/go/1.9/libexec/src/math/rand/rand.go:169 +0x38
  github.com/cenkalti/backoff.(*ExponentialBackOff).NextBackOff()
     /go/src/github.com/cenkalti/backoff/exponential.go:124 +0x10d
  main.main()
     /go/src/.../main.go:133 +0x52b

Previous write at 0x00c420099508 by goroutine 19:
  math/rand.(*rngSource).Int63()
      /usr/local/Cellar/go/1.9/libexec/src/math/rand/rng.go:241 +0xb9
  math/rand.(*Rand).Int63()
      /usr/local/Cellar/go/1.9/libexec/src/math/rand/rand.go:82 +0x51
  math/rand.(*Rand).Float64()
      /usr/local/Cellar/go/1.9/libexec/src/math/rand/rand.go:169 +0x38
  github.com/cenkalti/backoff.(*ExponentialBackOff).NextBackOff()
      /go/src/github.com/cenkalti/backoff/exponential.go:124 +0x10d
  github.com/cenkalti/backoff.(*backOffContext).NextBackOff()
      /go/src/github.com/cenkalti/backoff/context.go:58 +0xac
  github.com/cenkalti/backoff.(*Ticker).send()
      /go/src/github.com/cenkalti/backoff/ticker.go:74 +0x1a2
  github.com/cenkalti/backoff.(*Ticker).run()
      /go/src/github.com/cenkalti/backoff/ticker.go:48 +0xfa

Goroutine 19 (running) created at:
  github.com/cenkalti/backoff.NewTicker()
      /go/src/github.com/cenkalti/backoff/ticker.go:32 +0x1e1
  main.main()
      /go/src/.../main.go:131 +0xb6e
==================```

There are some great enhancements in master vs 2.0.0 that would be nice to include in a new version so tools like dep pull them in by default. Thanks!

This library is nice, but makes it awkward to classify errors as retriable or not.

The retriable function can either fail in a way that is retriable, or it can fail permanently. Permanent failure needs to be propagated, but since Retry() uses the error return to detect whether something should be retired, you need an additional error variable to track this. For example:

var fail error
err := backoff.Retry(func() error {
  resp := doSomeKindOfHttpRequest()
  if resp.Status == 404 {
    fail = errors.New("Not found")
    return nil  // Don't retry, though it failed
  }
  if resp.Status == 503 {
    return errors.New("Unavailable")  // Retry
  }
  return nil
}, backoff.NewExponentialBackOff())
if err != nil {
  return err
}
if fail != nil {
  return fail
}

The solution is to let Operation's signature be:

type Operation func() (err error, retriable bool)

...and only retry if it returns true. If it returns false, its error should be returned from Retry(). Rewritten example:

err := backoff.Retry(func() (error, bool) {
  resp := doSomeKindOfHttpRequest()
  if resp.Status == 404 {
    return errors.New("Not found"), false
  }
  if resp.Status == 503 {
    return errors.New("Unavailable"), true
  }
  return nil, false
}, backoff.NewExponentialBackOff())
if err != nil {
  return err
}

Another way would be to a have a pluggable "policy function" that can look at an error and determine if it's retriable, but that's probably too Javaesque for a simple library like this.

Although documented in the godocs, this behavior pushes more error handling into caller's code.

Most folks using RetryNotify() will have code that looks like the following:

if err := backoff.RetryNotify(op, backoff, handleError); err != nil {
    handleError(err)
}

Anyone wanting a function to be called when an error occurs, would also want it called when it is the final error.

After looking at the source code, this would require switching if statements in retry.go, and would not affect any tests. This would however be a breaking change, but would result in a more idiomatic behavior of the function.

Can a new version tag be added to 2ea60e5. A breaking change was introduced with the WithMaxRetries rename.

Hi

I would like to request the possibility to support contexts natively in this package, now that Go ships with a context package.

Contexts allow to cancel code as soon as we are not interested in the result anymore. For instance, if a client has a 120 seconds timeout, there is no reason to continue processing a request after 120 seconds.

The Retry and RetryNotify functions, however, will prevent cancellation for the duration specified in MaxElapsedTime. Even if the retried operation cancels early because it notices that some context is done, Retry/RetryNotify will sleep and call the operation again until MaxElapsedTime have actually elapsed.

Currently, the following solutions can be used:

• Setting MaxElapsedTime to a duration that matches the context's expected deadline. However, the deadline of a context isn't known (and not all contexts are deadline-based), so this solution is not practical.

• Using a mix of ticker and select:

  ticker := context.NewTicker(b)
  defer ticker.Cancel()
  
  outer_loop:
  for {
      select {
          case <-ticker.C:
              if err := op(); err == nil {
                  break outer_loop;
              }
          case <-ctx.Done():
              break outer_loop;
      }
  }
  

  However, this is cumbersome compared to using backoff.Retry()

It would be awesome if this package could support contexts natively.

One possibility would be to add a Context property on the backoff struct. This would be the only visible change to the interface, and would be fully backwards-compatible:

b := backoff.NewExponentialBackoff()
b.Context = ctx

context.Retry(op, b) // aborts as soon as context is done, or MaxElapsedTime is elapsed

This is fully backwards compatible, and doesn't complexifies the interface or the implementation.

Is there any goal to make the ExponentialBackOff implementation thread safe?

Similar to #19 and #22, I believe you should provide a mechanism to stop the backoff early. Certain errors are not retryable and, should these be encountered, the backoff should be stopped immediately, and the error returned to the caller for handling. Since you do not provide a mechanism to stop the backoff directly, the solution to #19 (wrap the operation function) is inadequate for this case. If you provide:

func ConditionalRetry(o Operation, c Conditional, b Backoff) error
func ConditionalRetryNotify(o Operation, c Conditional, n Notify, b Backoff) error

As suggested, you will be able to satisfy this use case AND address #22 without breaking backwards compatibility.

A ticker is inadequate since, as you specify in your documentation:

Ticks will continue to arrive when the previous operation is still running, so operations that take a while to fail could run in quick succession.

Clearly, there is a need for a blocking retry mechanism that can also abort in the case of an un-retryable error.

I have been trying to use ExponentialBackoff with an InitialInterval of 5*time.Second, but it continues to use the default of 500*time.Millisecond.

expBackoff := backoff.NewExponentialBackoff()
expBackoff.InitialInterval = 5*time.Second

With the first expBackoff.NextBackOff() that's logged:

DEBU[0000] Retrying login in 582.280027ms

This is similar to #62, but the difference is we now have a standard way to wrap errors that is backward-compatible.

Furthermore I think there is at least one case where wrapping might be useful. You might want to decouple calling backoff.Retry and the func itself so that the func might be called without backoff.Retry at all. Wrappable Permanent error can simplify this case considerably.

code: https://github.com/cenkalti/backoff/blob/master/retry.go#L52

go doc: https://golang.org/pkg/time/#NewTicker

Maybe we could just use time.After.

I noticed that the Ticker API can have multiple invocations of the target function active at a given time when the target function's execution takes more time than the current retry interval.

The Retry API however does not behave in the same way - it will always wait for the previous execution to finish before executing the next iteration.

Is this on purpose? If so it would be helpful to clarify this behaviour in the docs, if not a code change would be required. Let me know what you think.

Specifying in the go.mod file

require github.com/cenkalti/backoff v3.0.0

and running go mod tidy will correct the previous line to

require github.com/cenkalti/backoff v0.0.0-20190511092443-4b4cebaf850e

and leads to the error message by go mod tidy

go: finding github.com/cenkalti/backoff v3.0.0
go: github.com/cenkalti/[email protected]: go.mod has post-v0 module path "github.com/cenkalti/backoff/v3" at revision 4b4cebaf850e
go: error loading module requirements

What if the func to retry is returning values or updating a variable which needs to be send in?

The example below is simple and shows with logs what's happening. I think it's better if we replace the example in the docs with this. I'll open PR when I have time, for now I just paste it here:

package main

import (
    "fmt"
    "log"

    "github.com/cenkalti/backoff"
)

func main() {
    if err := realMain(); err != nil {
        log.Fatalln(err)
    }
}

func realMain() error {
    count := 0

    operation := func() error {
        if count != 10 {
            count++
            log.Printf("Count is '%d'. Needs to be '10'", count)
            return fmt.Errorf("Not finished")
        }

        log.Println("Operation succedded")
        return nil
    }

    log.Println("Calling backoff.Retry")
    err := backoff.Retry(operation, backoff.NewExponentialBackOff())
    if err != nil {
        // Handle error.
        return err
    }

    log.Println("Operation is finished")
    return nil
}

package main

import (
    "fmt"

    "github.com/cenkalti/backoff"
)

func main() {
    b := backoff.NewExponentialBackOff()

    fmt.Println(b.NextBackOff())
    fmt.Println(b.NextBackOff())
    fmt.Println(b.NextBackOff())
    fmt.Println(b.NextBackOff())
    fmt.Println(b.NextBackOff())
}

Output:

-1ns
-1ns
-1ns
-1ns
-1ns

~/demo  go version
go version go1.4.2 darwin/amd64

Currently, calling Retry(f, b) will call f twice if the Backoff b is created with WithMaxTries(1), which seems somewhat counter-intuitive.

https://github.com/cenkalti/backoff/blob/master/tries.go#L24

This line should be if b.numTries < b.maxTries {?

Code:

package main

import (
	"errors"
	"fmt"
	"time"

	"github.com/cenkalti/backoff"
)

func main() {
	bo := &backoff.ExponentialBackOff{
		InitialInterval:     500 * time.Millisecond,
		RandomizationFactor: 0.5,
		Multiplier:          1.5,
		MaxInterval:         60 * time.Second,
		MaxElapsedTime:      5 * time.Minute,
		Clock:               backoff.SystemClock,
	}

	op := func() error { return errors.New("kaboom") }
	if err := backoff.Retry(op, bo); err != nil {
		fmt.Println("error", err)
	}
}

What I expect to see

The program should hang until MaxElapsedTime passes, and then print an error.

What I see instead

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x108bb52]

goroutine 1 [running]:
math/rand.(*Rand).Int63(0x0, 0xc420012140)
	/usr/local/go/src/math/rand/rand.go:81 +0x22
math/rand.(*Rand).Float64(0x0, 0x10c0d30)
	/usr/local/go/src/math/rand/rand.go:168 +0x2b
github.com/cenkalti/backoff.(*ExponentialBackOff).NextBackOff(0xc4200160c0, 0x0)
	/Users/ivan/workspace/Go/src/github.com/cenkalti/backoff/exponential.go:121 +0x6f
github.com/cenkalti/backoff.RetryNotify(0x10c0d38, 0x1114340, 0xc4200160c0, 0x0, 0x1133fa8, 0x10a3b80)
	/Users/ivan/workspace/Go/src/github.com/cenkalti/backoff/retry.go:45 +0xe1
github.com/cenkalti/backoff.Retry(0x10c0d38, 0x1114340, 0xc4200160c0, 0x1133fa8, 0x0)
	/Users/ivan/workspace/Go/src/github.com/cenkalti/backoff/retry.go:25 +0x48
main.main()
	/tmp/backoff/main.go:22 +0xe1
exit status 2

This is caused by the following line [1]:

	return getRandomValueFromInterval(b.RandomizationFactor, b.random.Float64(), b.currentInterval)

since b.random is nil.

Am I misusing the package? In your test, you set all of ExponentialBackOff fields after instantiating the struct with NewExponentialBackOff [2], but this seems weird to me.

Adding the following lines to (*ExponentialBackOff).NextBackOff should solve the issue (and make the ExponentialBackOff type much more usable, IMHO):

	if b.random == nil {
		b.random = rand.New(rand.NewSource(time.Now().UnixNano()))
	}

I could send a PR if you're ok with the proposed solution.
Thanks,
Ivan

[1] https://github.com/cenkalti/backoff/blob/master/exponential.go#L121
[2] https://github.com/cenkalti/backoff/blob/master/exponential_test.go#L18-L23

Hello! First of all, thanks for this library, it's a really nice balance of simplicity and features!

I am running in to an issue when using the Ticker with a select loop and I'm convinced I'm doing something stupid.

Here's a quick example taken from the range loop example:

func main() {
	// An operation that may fail.
	operation := func() error {
		return fmt.Errorf("failed")
	}

	b := backoff.NewExponentialBackOff()
	b.MaxElapsedTime = 10 * time.Second // I want to limit the maximum time it will wait to 10 seconds
	ticker := backoff.NewTicker(b)

	var err error
	for {
		select {
		case t := <-ticker.C:
			if err = operation(); err != nil {
				log.Printf("ticker time: %s\tNext tick: %s\n", t, b.NextBackOff())
                                time.Sleep(1 * time.Second)
			}
		}
	}
	// Operation is successful.
}

What I want to have happen is for the Ticker to stop sending timestamps when it exceeds MaxElapsedTime, instead I am seeing a massive spam of zero timestamps and a NextBackOff of -1ns

go build && ./test
2019/11/30 09:43:50 ticker time: 2019-11-30 09:43:50.712964693 +1300 NZDT m=+0.000274499        Next tick: 1.080381816s
2019/11/30 09:43:51 ticker time: 2019-11-30 09:43:51.265902607 +1300 NZDT m=+0.553212487        Next tick: 1.31013006s
2019/11/30 09:43:53 ticker time: 2019-11-30 09:43:53.296598454 +1300 NZDT m=+2.583908309        Next tick: 4.506218855s
2019/11/30 09:43:55 ticker time: 2019-11-30 09:43:55.637339103 +1300 NZDT m=+4.924648958        Next tick: 5.608623477s
2019/11/30 09:43:58 ticker time: 2019-11-30 09:43:58.858993827 +1300 NZDT m=+8.146303723        Next tick: 15.394871241s
2019/11/30 09:44:06 ticker time: 2019-11-30 09:44:06.509425175 +1300 NZDT m=+15.796735081       Next tick: -1ns
2019/11/30 09:44:07 ticker time: 0001-01-01 00:00:00 +0000 UTC  Next tick: -1ns
2019/11/30 09:44:08 ticker time: 0001-01-01 00:00:00 +0000 UTC  Next tick: -1ns
2019/11/30 09:44:09 ticker time: 0001-01-01 00:00:00 +0000 UTC  Next tick: -1ns
2019/11/30 09:44:10 ticker time: 0001-01-01 00:00:00 +0000 UTC  Next tick: -1ns

Any pointers greatly appreciated!

The constructor for an exponential backoff does calls to rand.New, rand.NewSource, and time.Now.

If a user of this library tries to use many concurrent backoffs these calls significantly affect the performance of the program.

Because the random field is not exported a user of this library cannot create their own instance of rand and use that to reduce the number of calls to the mentioned methods.

https://github.com/cenkalti/backoff/blob/master/exponential.go#L92

func NewExponentialBackOff() *ExponentialBackOff {
	b := &ExponentialBackOff{
		InitialInterval:     DefaultInitialInterval,
		RandomizationFactor: DefaultRandomizationFactor,
		Multiplier:          DefaultMultiplier,
		MaxInterval:         DefaultMaxInterval,
		MaxElapsedTime:      DefaultMaxElapsedTime,
		Clock:               SystemClock,
		random:              rand.New(rand.NewSource(time.Now().UnixNano())),
	}
	b.Reset()
	return b
}

Hello,

Can you please tag and version this project?

I am the Debian Maintainer for backoff and versioning would help Debian keep up with development.

I often stumble upon projects are using the https://github.com/azr/backoff/ fork. I wish those commits are brought here.

This won't work if the error is wrapped into say pkg/errors error (https://github.com/pkg/errors) or any other custom error. Wrapping these into PermanentError is even worse as no original error can be recovered.

This is pretty dangerous, as PermanentError will be retried in that case.
Also it's hard to detect until you dig into the lib source and see that line above.

More details:
Typically you wrap you errors at the entry point to your code

func Post(...) error {
        res, err := client.Do(req)  // this is the error's entry point to your code
	if err != nil {
		return errors.Wrap(err, "bad call")  // wrap it with stacktrace etc.
	}
        if err := foo(...);  err != nil {
                 return backoff.Permanent(err)  // this one needs to be wrapped, but then it won't be PermanentError anymore
        }
}

func RetriablePost(...) error {
	operation := func() error {
		return Post(...)
	}
	err := backoff.Retry(operation, backoff.NewExponentialBackOff())
	return err
}

Instead it's better to return an explicit indicator of terminal error

	operation := func() (bool, error) {
	}

@cenkalti I want to update Go version in Travis file from 1.3.3 to 1.x(latest stable) or more explicit 1.9.x ?

The retry logic currently relies on the time package to create a concrete *time.Ticker to retry, and there's no way to replace that ticker with a mocked one. There's a way to replace the SystemClock with another clock, but that's not sufficient to completely avoid interaction with system time.

When testing code that uses backoff mechanisms, it's important to be able to specify precise timing scenarios, while running at full CPU speed, to obtain reliable determinisitic, non-flaky results.

I propose:

• replacing direct usage of the time.Ticker type with an indirect usage, via an additional interface
• implementing a method to provide a custom implementation of that interface

I'm opening this issue to see what you think about it. I'll follow up with a PR if you are interested.

Here you say

It is the caller's responsibility to reset b after Retry returns.

but here you reset it for them before attempting the retry.

Is there a different reason I'd need to reset the backoff after return? Or are the docs wrong, or is the code?

Thanks.

Although this is very clearly documented in the documentation for Retry, I feel that the operation should not be called if the provided context.Context is already canceled. This has the potential to needlessly call a resource intensive function.

This is a backwards incompatible change, so the library should be bumped to v3 if you feel that this is worth the change

Using the default system rand.Float64() will return the same values in the same order over subsequent invocations.

There is much written about this, but if you invoke this (https://play.golang.org/p/_b5JQHun3M) several times you will see the output never varies from:

0.604660
0.940509
0.664560
0.437714
0.424637
0.686823
0.065637
0.156519
0.096970
0.300912

And per the docs (https://golang.org/pkg/math/rand/#example__rand):

// Typically a non-fixed seed should be used, such as time.Now().UnixNano().
// Using a fixed seed will produce the same output on every run.

PR incoming.

What is the issue?

The example of:

// An operation that may fail.
operation := func() error {
    return nil // or an error
}

err := Retry(operation, NewExponentialBackOff())
if err != nil {
    // Handle error.
    return
}

// Operation is successful.

Found at: https://godoc.org/github.com/cenkalti/backoff#example-Retry

Does not work even when you correctly prefix by package e.g:

// An operation that may fail.
operation := func() error {
    return nil // or an error
}

err := backoff.Retry(operation, backoff.NewExponentialBackOff())
if err != nil {
    // Handle error.
    return
}

// Operation is successful.

Why?

NewExponentialBackOff does not implement NextBackOff

Add a set of functions such as

func ConditionalRetry(o Operation, c Conditional, b Backoff) error
func ConditionalRetryNotify(o Operation, c Conditional, n Notify, b Backoff) error

where

func Conditional(e error) bool

The idea being that some operations return errors, and you only want to backoff on certain errors. Otherwise you just want to drop the error through to the receiver of ConditionalRetry or ConditionalRetryNotify

I already implemented a hacky version of this for a project that I needed to have that functionality. If there's interest of supporting this I could clean up what I have and throw up a PR

many open source go projects use this nowadays: https://coveralls.io/

Sometimes, we want to back off exponentially, but rather than setting a max time limit, it would be nice to be able to set a maximum number of tries instead.

Something I noticed while writing a unit test is that a backoff I created as backoff.WithMaxRetries(&backoff.ZeroBackOff{}, 0) did not stop retrying after the first attempt failed, and kept attempting to retry.

This is a bit different than what I expected, since I figured a max retry amount of zero would cause there to be no retries at all.

Is this expected?

Hi

Sometimes the retries operation knows that it's not going to succeed, ever. For instance, if http.NewRequest() fails because the URL is malformed, or if a server returns a 400 Bad Request, retrying will probably never succeed.

It would be great if there was a way for the operation to specify that it shouldn't be retried.

I would like to propose an implementation of this that doesn't change the API of the package (no changes, no additions), and keeps the implementation simple.

One possible way of doing this would be to allow the operation to return a particular implementation of error. Retry() would stop retrying if the operation returned this error:

package backoff

type PermanentError struct {
    Err error
}

func (e *PermanentError) String() {
    e.Err.String()
}

// In Retry():

if err = operation(); err == nil {
	return nil
}

if err, ok := err.(*PermanentError); ok {
    return err.Err
}

And in user code:

backoff.Retry(func() {
    req, err := http.NewRequest("GET", "http://example com", nil)
    if err != nil {
        return &backoff.PermanentError{err}
    }
}, b)