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.

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

Zero-value retrier will break your stuff. #7

Closed
flowchartsman opened this issue Dec 16, 2020 · 0 comments
Closed

Zero-value retrier will break your stuff. #7

flowchartsman opened this issue Dec 16, 2020 · 0 comments

Comments

@flowchartsman
Copy link
Owner

It's been awhile since I wrote this module and, looking at it again while making it an actual module, I note a glaring problem: it allows you to create a zero-value Retrier, whereupon it will panic:

package main

import (
	"errors"
	"github.com/flowchartsman/retry"
)

func main() {
	r := retry.Retrier{}
	r.Run(func() error {
		return errors.New("nope")
	})
}

Results in

panic: invalid argument to Int63n

goroutine 1 [running]:
math/rand.(*Rand).Int63n(0xc000074090, 0x0, 0x0)
        /Users/awalker/opt/go/src/math/rand/rand.go:111 +0x11d
github.com/flowchartsman/retry.jitterDuration(0x0, 0x0)
        /Users/awalker/go/pkg/mod/github.com/flowchartsman/retry@v1.1.0/retry.go:126 +0x87
github.com/flowchartsman/retry.getnextBackoff(0x1, 0x0, 0x0, 0xc0000100f0)
        /Users/awalker/go/pkg/mod/github.com/flowchartsman/retry@v1.1.0/retry.go:112 +0x65
github.com/flowchartsman/retry.(*Retrier).RunContext(0xc000104f60, 0x10a36c0, 0xc0000140a0, 0xc000104f20, 0x16510f9d643f60d8, 0xc00004e748)
        /Users/awalker/go/pkg/mod/github.com/flowchartsman/retry@v1.1.0/retry.go:81 +0x7d
github.com/flowchartsman/retry.(*Retrier).Run(0xc00004e760, 0x108f770, 0x0, 0x0)
        /Users/awalker/go/pkg/mod/github.com/flowchartsman/retry@v1.1.0/retry.go:45 +0x6b
main.main()
        /Users/awalker/go/src/scratch/retrybug/main.go:11 +0x48

Because, as we have seen before values <=0 passed to rand.Int63 will fail, and a zero-value Retrier is, unsurprisingly, filled with zeros.

Looking back on it, I can see what I was going for: I was attempting to force the user to create an object that could be messed with after instantiation, thus making it safe to use concurrently as generator at runtime. A good idea, but allowing a panic condition is a poor oversight that's easy to hit, since I just found myself attempting to do so, and others doubtless will as well. I think the simplest fix is to add the same default checks that are in place in the NewRetrier() method in the `RunContext()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant