Skip to content

Commit 4817c77

Browse files
authored
Rewrite an irrational test which changes behavior based on compiler inlining (#1172)
This test is rather confusingly / greatly over-complicated anyway, as written. It's passing an "ignore these errors" func, and then ignoring a pointer to the same error *content* as is being returned elsewhere... by passing it through a closure and a loop of `==` comparisons on interface-boxed values. Instead of just `return false`. Bleh. --- More interestingly though, TestIsRetryableFailure runs fine with `make unit_test`, as our green CI pipeline shows. However, when you run it by hand: ``` ❯ go test ./internal/common/backoff --- FAIL: TestIsRetryableFailure (0.01s) retry_test.go:150: Error Trace: retry_test.go:150 Error: An error is expected but got nil. Test: TestIsRetryableFailure retry_test.go:151: Error Trace: retry_test.go:151 Error: Not equal: expected: 1 actual : 5 Test: TestIsRetryableFailure FAIL FAIL go.uber.org/cadence/internal/common/backoff 0.572s FAIL ``` After digging around a bit, I noticed that it passed when both `-race` and `-coverprofile` were passed (as `make unit_test` does), but not with either (or none) were passed. Adding a print statement to `IgnoreErrors` also caused the test to pass. Once the print statement came into play, I figured it had to be due to optimizations of some kind, so I poked around with `-gcflags -m` and the most obvious difference with the print statement was that it prevented inlining of `IgnoreErrors` and the anonymous func in `TestIsRetryableFailure`. As further evidence: ``` # default inlining ❯ go test -gcflags -l=0 ./internal/common/backoff --- FAIL: TestIsRetryableFailure (0.01s) retry_test.go:150: Error Trace: retry_test.go:150 Error: An error is expected but got nil. Test: TestIsRetryableFailure retry_test.go:151: Error Trace: retry_test.go:151 Error: Not equal: expected: 1 actual : 5 Test: TestIsRetryableFailure FAIL FAIL go.uber.org/cadence/internal/common/backoff 0.572s FAIL # no inlining ❯ go test -gcflags -l=1 ./internal/common/backoff ok go.uber.org/cadence/internal/common/backoff 0.494s ``` You can also see the differences between `-race` +/- `-coverprofile` optimization with `-gcflags -m` if you're interested in more detail. --- The underlying reason for all this is that, as the go spec notes: https://go.dev/ref/spec#Comparison_operators > Pointers to distinct zero-size variables may or may not be equal. And `someError` is a zero-size type. When combined with general inlining behavior getting better and better: https://dave.cheney.net/2020/05/02/mid-stack-inlining-in-go you get inconsistent behavior depending on optimization level. Fun! The good news is that this is probably a test-only concern in this case.
1 parent ae97d02 commit 4817c77

File tree

2 files changed

+8
-25
lines changed

2 files changed

+8
-25
lines changed

internal/common/backoff/retry.go

Lines changed: 0 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -123,16 +123,3 @@ Retry_Loop:
123123
time.Sleep(next)
124124
}
125125
}
126-
127-
// IgnoreErrors can be used as IsRetryable handler for Retry function to exclude certain errors from the retry list
128-
func IgnoreErrors(errorsToExclude []error) func(error) bool {
129-
return func(err error) bool {
130-
for _, errorToExclude := range errorsToExclude {
131-
if err == errorToExclude {
132-
return false
133-
}
134-
}
135-
136-
return true
137-
}
138-
}

internal/common/backoff/retry_test.go

Lines changed: 8 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,6 @@ import (
2929
"github.com/stretchr/testify/assert"
3030
)
3131

32-
type someError struct{}
33-
3432
func TestRetrySuccess(t *testing.T) {
3533
t.Parallel()
3634
i := 0
@@ -112,19 +110,13 @@ func TestIsRetryableSuccess(t *testing.T) {
112110
return &someError{}
113111
}
114112

115-
isRetryable := func(err error) bool {
116-
if _, ok := err.(*someError); ok {
117-
return true
118-
}
119-
120-
return false
121-
}
122-
123113
policy := NewExponentialRetryPolicy(1 * time.Millisecond)
124114
policy.SetMaximumInterval(5 * time.Millisecond)
125115
policy.SetMaximumAttempts(10)
126116

127-
err := Retry(context.Background(), op, policy, isRetryable)
117+
err := Retry(context.Background(), op, policy, func(err error) bool {
118+
return true // retry on any error
119+
})
128120
assert.NoError(t, err, "Retry count: %v", i)
129121
assert.Equal(t, 5, i)
130122
}
@@ -146,7 +138,9 @@ func TestIsRetryableFailure(t *testing.T) {
146138
policy.SetMaximumInterval(5 * time.Millisecond)
147139
policy.SetMaximumAttempts(10)
148140

149-
err := Retry(context.Background(), op, policy, IgnoreErrors([]error{&someError{}}))
141+
err := Retry(context.Background(), op, policy, func(err error) bool {
142+
return false // never retry
143+
})
150144
assert.Error(t, err)
151145
assert.Equal(t, 1, i)
152146
}
@@ -198,6 +192,8 @@ func TestConcurrentRetrier(t *testing.T) {
198192
}
199193
}
200194

195+
type someError struct{}
196+
201197
func (e *someError) Error() string {
202198
return "Some Error"
203199
}

0 commit comments

Comments
 (0)