uber-go / cadence-client

Framework for authoring workflows and activities running on top of the Cadence orchestration engine.
https://cadenceworkflow.io
MIT License
344 stars 130 forks source link

Rewrite an irrational test which changes behavior based on compiler inlining #1172

Closed Groxx closed 2 years ago

Groxx commented 2 years ago

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.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 01816de7-8921-41ec-a3bc-fcec6690e6d2


Totals Coverage Status
Change from base Build 018168d3-2407-4854-a083-a91b90cf348f: -0.008%
Covered Lines: 12391
Relevant Lines: 19432

💛 - Coveralls