mock: avoid data races when expecting calls #1693
Conversation
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. Signed-off-by: Peter Gardfjäll <[email protected]> Signed-off-by: Johannes Würbach <[email protected]>
johanneswuerbach
force-pushed
the
mockdatarace
branch
from
87075bf to
5d0bea5
Compare
|
I just got a race condition error on my tests and this patch fixes it |
|
This fixes #1597 -- when it will be available? |
|
Any news about this PR? |
There was a problem hiding this comment.
You haven't used the pull request template, so I've had to piece together the approach. Please use the template so that I know I've not missed or misunderstood anything.
Your approach uses the fact that Mock.Called calls Arguments.Diff initially through mock.findExpectedCalls to work out if the call is expected, this doesn't need to format the args at all. Then if the call was unexpected it will call Arguments.Diff again through Mock.findClosestCall which does need to format the arguments. You've provided Arguments.Diff with an operational or no-op formatter to stop it from rendering on the first path.
This;
- prevents data races caused by reads in fmt.Sprintf formatting the actual argument values when the call is expected.
- does not prevent data races caused by reads in fmt.Sprintf formatting the actual argument values when the call is not expected.
- does not prevent data races caused by reads in reflect.DeepEqual comparing the actual argument values to the expected values regardless of whether the call is expected or not.
Basically this only prevents races on values checked against mock.Anything or ignored by a mock.MatchedBy func. This does not close #1597 but is a sufficiently common problem for people to have that I think I'm open to merging this change.
| } | ||
|
|
||
| // diff allows for the diffing of arguments and objects. | ||
| func (args Arguments) diff(objects []interface{}, includeOutput bool) (string, int) { |
There was a problem hiding this comment.
I'm not a fan of data (bool) arguments changing the behaviour of functions. You can just pass the behaviour instead:
| func (args Arguments) diff(objects []interface{}, includeOutput bool) (string, int) { | |
| func (args Arguments) diff(objects []interface{}, fmtArg func(format string, args ...interface{})) (string, int) { |
Instead of modifying the output as proposed in #1598, avoid any kind of output when trying to match calls.