From 5643445eeef62283311d3a89541d609ae40bc757 Mon Sep 17 00:00:00 2001 From: Sung Yoon Whang Date: Tue, 24 Oct 2023 09:00:56 -0700 Subject: [PATCH] Revert "Make maxSleep and maxRetires configurable when building options (#94)" (#116) This reverts commit 751da596f6f726bcc02401452bacb0fda8fc7b7b. Per discussion in #93, we don't want to release this change as-is. --- leaks.go | 3 --- leaks_test.go | 12 +----------- options.go | 44 +++++++------------------------------------- options_test.go | 16 +++------------- 4 files changed, 11 insertions(+), 64 deletions(-) diff --git a/leaks.go b/leaks.go index ccb50b4..cc206f1 100644 --- a/leaks.go +++ b/leaks.go @@ -56,9 +56,6 @@ func Find(options ...Option) error { cur := stack.Current().ID() opts := buildOpts(options...) - if err := opts.validate(); err != nil { - return err - } if opts.cleanup != nil { return errors.New("Cleanup can only be passed to VerifyNone or VerifyTestMain") } diff --git a/leaks_test.go b/leaks_test.go index 7db46a7..992c85b 100644 --- a/leaks_test.go +++ b/leaks_test.go @@ -36,7 +36,7 @@ var _ = TestingT(testing.TB(nil)) // testOptions passes a shorter max sleep time, used so tests don't wait // ~1 second in cases where we expect Find to error out. func testOptions() Option { - return MaxSleepInterval(time.Millisecond) + return maxSleep(time.Millisecond) } func TestFind(t *testing.T) { @@ -60,16 +60,6 @@ func TestFind(t *testing.T) { err := Find(Cleanup(func(int) { assert.Fail(t, "this should not be called") })) require.Error(t, err, "Should exit with invalid option") }) - - t.Run("Find should return error when maxRetries is less than 0", func(t *testing.T) { - err := Find(MaxRetryAttempts(-1)) - require.Error(t, err, "maxRetries should be greater than 0") - }) - - t.Run("Find should return error when maxSleep is less than 0s", func(t *testing.T) { - err := Find(MaxSleepInterval(time.Duration(-1))) - require.Error(t, err, "maxSleep should be greater than 0s") - }) } func TestFindRetry(t *testing.T) { diff --git a/options.go b/options.go index ad5e77b..53fc0a1 100644 --- a/options.go +++ b/options.go @@ -21,7 +21,6 @@ package goleak import ( - "errors" "strings" "time" @@ -33,14 +32,10 @@ type Option interface { apply(*opts) } -const ( - // We retry up to default 20 times if we can't find the goroutine that - // we are looking for. - _defaultRetryAttempts = 20 - // In between each retry attempt, sleep for up to default 100 microseconds - // to let any running goroutine completes. - _defaultSleepInterval = 100 * time.Microsecond -) +// We retry up to 20 times if we can't find the goroutine that +// we are looking for. In between each attempt, we will sleep for +// a short while to let any running goroutines complete. +const _defaultRetries = 20 type opts struct { filters []func(stack.Stack) bool @@ -58,17 +53,6 @@ func (o *opts) apply(opts *opts) { opts.cleanup = o.cleanup } -// validate the options. -func (o *opts) validate() error { - if o.maxRetries < 0 { - return errors.New("maxRetryAttempts should be greater than 0") - } - if o.maxSleep <= 0 { - return errors.New("maxSleepInterval should be greater than 0s") - } - return nil -} - // optionFunc lets us easily write options without a custom type. type optionFunc func(*opts) @@ -123,25 +107,12 @@ func IgnoreCurrent() Option { }) } -// MaxSleepInterval sets the maximum sleep time in-between each retry attempt. -// The sleep duration grows in an exponential backoff, to a maximum of the value specified here. -// If not configured, default to 100 microseconds. -func MaxSleepInterval(d time.Duration) Option { +func maxSleep(d time.Duration) Option { return optionFunc(func(opts *opts) { opts.maxSleep = d }) } -// MaxRetryAttempts sets the retry upper limit. -// When finding extra goroutines, we'll retry until all goroutines complete -// or end up with the maximum retry attempts. -// If not configured, default to 20 times. -func MaxRetryAttempts(num int) Option { - return optionFunc(func(opts *opts) { - opts.maxRetries = num - }) -} - func addFilter(f func(stack.Stack) bool) Option { return optionFunc(func(opts *opts) { opts.filters = append(opts.filters, f) @@ -150,8 +121,8 @@ func addFilter(f func(stack.Stack) bool) Option { func buildOpts(options ...Option) *opts { opts := &opts{ - maxRetries: _defaultRetryAttempts, - maxSleep: _defaultSleepInterval, + maxRetries: _defaultRetries, + maxSleep: 100 * time.Millisecond, } opts.filters = append(opts.filters, isTestStack, @@ -162,7 +133,6 @@ func buildOpts(options ...Option) *opts { for _, option := range options { option.apply(opts) } - return opts } diff --git a/options_test.go b/options_test.go index bb2fe06..0b437ea 100644 --- a/options_test.go +++ b/options_test.go @@ -88,20 +88,10 @@ func TestOptionsIgnoreAnyFunction(t *testing.T) { } } -func TestBuildOptions(t *testing.T) { - // With default options. - opts := buildOpts() - assert.Equal(t, _defaultSleepInterval, opts.maxSleep, "value of maxSleep not right") - assert.Equal(t, _defaultRetryAttempts, opts.maxRetries, "value of maxRetries not right") - - // With customized options. - opts = buildOpts(MaxRetryAttempts(50), MaxSleepInterval(time.Microsecond)) - assert.Equal(t, time.Microsecond, opts.maxSleep, "value of maxSleep not right") - assert.Equal(t, 50, opts.maxRetries, "value of maxRetries not right") -} - func TestOptionsRetry(t *testing.T) { - opts := buildOpts(MaxSleepInterval(time.Millisecond), MaxRetryAttempts(50)) // initial attempt + 50 retries = 51 + opts := buildOpts() + opts.maxRetries = 50 // initial attempt + 50 retries = 11 + opts.maxSleep = time.Millisecond for i := 0; i < 50; i++ { assert.True(t, opts.retry(i), "Attempt %v/51 should allow retrying", i)