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

Add ForAttempt #5

Merged
merged 1 commit into from
Mar 1, 2016
Merged

Add ForAttempt #5

merged 1 commit into from
Mar 1, 2016

Conversation

keegancsmith
Copy link
Contributor

See documentation for motivation.

See documentation for motivation.
@keegancsmith
Copy link
Contributor Author

In my case I want to be conservative with my memory usage. I can understand if this feature doesn't make sense for upstream given the simplicity of the current API.

@jpillora
Copy link
Owner

jpillora commented Mar 1, 2016

LGTM 👍

jpillora added a commit that referenced this pull request Mar 1, 2016
@jpillora jpillora merged commit d342b63 into jpillora:master Mar 1, 2016
@DrTall
Copy link
Contributor

DrTall commented Apr 6, 2016

Howdy,

I think this was a good idea and I'm looking to use it in a project, but the section where the defaults get applied (if b.Min == 0 { ...) isn't thread safe. It makes sense that the regular use case with Duration / Reset isn't threadsafe, but it is kind of a shame that ForAttempt is also not threadsafe.

I'm not sure what the best way to fix is. The projects at my company use the pattern of setting defaults via constructor, e.g.

func NewBackoffConfig() *backoffConfig {
return backoffConfig{
  Min: 100 * time.Millisecond,
  Max: 10 * time.Second,
  Factor: 2
}
}

// User code example
config := NewBackoffConfig()
config.Max = 30 * time.Second
backoff := NewBackoff(config)

And then you don't bother checking for invalid config values later on. Of course this would break existing code so it might not be doable here.

Alternatively, ForAttempt could just use the default values locally and not write them back into b. That is, basically make ForAttemptinto a value receiver method instead of pointer receiver so it isn't racy.

Any thoughts?

Thanks,
Sean

@jpillora jpillora mentioned this pull request Apr 6, 2016
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

Successfully merging this pull request may close these issues.

None yet

3 participants