stretchr
mock: avoid data races in Arguments.Diff
Fixes a concurrency issue that would lead to testify mocks producing data races detected by go test -race. These data races would occur whenever a mock pointer argument was concurrently modified. The reason being that Arguments.Diff uses the %v format specifier to get a presentable string for the argument. This also traverses the pointed-to data structure, which would lead to the data race.
It should be noted that it is heavily inspired by https://github.com/stretchr/testify/pull/1229, but since that PR hasn't seen any updates in over a year I decided to start fresh.
Summary
Avoids testify mocks causing data races for (concurrently accessed) pointer arguments when running go test -race.
Changes
Arguments.Diff now uses %p to produce a "presentable string" for pointers rather than %v, since the latter traverses the pointed to data structure.
Motivation
We need to test our code for race conditions with go test -race, and we cannot have testify be the offender of data races.
Related issues
https://github.com/stretchr/testify/issues/1597
Thinking more on the above, does IsPtr need to recurse deep objects up to the first pointer shaped object to catch pointer fields in non-pointer structs? It's turtles all the way down.
Thinking more on the above, does
IsPtrneed to recurse deep objects up to the first pointer shaped object to catch pointer fields in non-pointer structs? It's turtles all the way down.
Yes, I think you are right. The IsPtr only works for pointer arguments, but not for other "mutable" types such as Map, Slice, Array. So a more comprehensive check would be something like:
// isMutable indicates if the supplied value is of a mutable type.
func isMutable(v interface{}) bool {
switch reflect.ValueOf(v).Kind() {
case reflect.Array, reflect.Chan, reflect.Map, reflect.Ptr, reflect.Slice:
return true
}
return false
}
And then it still won't catch the case where a struct value (not pointer) gets passed which contains a nested pointer field that gets updated.
Not sure how to solve this as recursing down v in search of a mutable/"pointerish" field traverses the value which is what we were trying to avoid to begin with (then we might as well just use %v). :shrug:
It is possible to write a test to show if any leaf of the structure is "pointerish" without reading those leaves, but I like it about as much as you do.
I considered if we could make a deep copy of the object rather than relying on the pointers but this isn't possible. Even with reflection you cannot copy unexported struct fields, but unexported struct fields' values are used when comparing structs with reflect.DeepEqual (and with ==).
Would it be an option to stringify the expect and actual args only in case of an error or and when the output is actually being used?
E.g. when using a "count only" diff in places like https://github.com/stretchr/testify/blob/master/mock/mock.go#L378-L384 the data race seems also to be resolved.
I PoC-ed the idea here https://github.com/stretchr/testify/pull/1693 and it seems to work for us.
