mock: reduce data races in Arguments.Diff for pointer-like arguments#1866
mock: reduce data races in Arguments.Diff for pointer-like arguments#1866pavelzeman wants to merge 1 commit intostretchr:masterfrom
Conversation
Arguments.Diff uses fmt.Sprintf("%v") to format arguments for diff output.
When arguments contain pointers, maps, slices, or other reference types that
are concurrently modified by other goroutines, this causes data races:
- For maps: a fatal "concurrent map iteration and map write" error that
crashes the entire test process (non-recoverable)
- For pointers/slices: data races detectable by -race, and potential panics
This commit introduces safeFormatArg() which uses %p (address only) for
reference types (pointers, maps, slices, channels) instead of %v, avoiding
traversal of the underlying data structure entirely. All other types use
the full %v representation since they are value types and cannot race.
Fixes stretchr#1597
|
Related commit in project Mattermost's testsuite: mattermost/mattermost@4b8a4ae Previous closed PR: #1865 |
dolmen
left a comment
dolmen
left a comment
There was a problem hiding this comment.
The submission claims "Fixes #1594", but this isn't the case: some cases of races are avoided in formatting the output (so this PR would still be an improvement), but races could also happen in the comparison itself.
So the claims in the commit message and the description of this PR must be lowered.
Some test cases are added that show the issue when the diff output is formatted, but they cover just Arguments.Diff, and more tests that cover the full call from Mock would be welcome.
Once my specific issues are fixed (ex: missing testsuite for safeFormatArg, renamming the function...) we will probably be able to merge this, but #1594 will still not be fixed.
| // cannot race, so they use the full %v representation. | ||
| // | ||
| // See https://github.com/stretchr/testify/issues/1597 | ||
| func safeFormatArg(v interface{}) string { |
There was a problem hiding this comment.
We need a testsuite for this new function.
Also, in its current implementation it is safer but no yet "safe". So the name is misleading.
| return fmt.Sprintf("(%[1]T=%[1]v)", v) | ||
| } | ||
| switch reflect.TypeOf(v).Kind() { | ||
| case reflect.Ptr, reflect.Map, reflect.Slice, reflect.Chan: |
There was a problem hiding this comment.
Replace reflect.Ptr (deprecated) with reflect.Pointer.
Other types must also be handled. In particular reflect.Array, reflect.Interface. We definitely need a comprehensive testsuite.
| } | ||
| switch reflect.TypeOf(v).Kind() { | ||
| case reflect.Ptr, reflect.Map, reflect.Slice, reflect.Chan: | ||
| return fmt.Sprintf("(%T=%p)", v, v) |
There was a problem hiding this comment.
Have you checked the output of safeFormatArg([]int(nil))?
We need a comprehensive test suite.
Also use [1] like in other paths.
| } | ||
|
|
||
| // Test_Arguments_Diff_ConcurrentPointerModification verifies that | ||
| // Arguments.Diff does not race when a pointer argument is concurrently |
There was a problem hiding this comment.
The 3 tests have much common code. Please refactor to extract a func(t *testing.T, arg interface{}, accessArg func()).
Also group them as a top level test with 3 subtests.
dolmen
changed the title
2 participants
Summary
Fixes
Arguments.Diffcausing data races when mock arguments (pointers, maps, slices) are concurrently modified by other goroutines.Problem
Arguments.Diffusesfmt.Sprintf("%v")to format arguments for diff output. This traverses the underlying data structure, which races with concurrent writers:fatal error: concurrent map iteration and map write— crashes the entire test process (non-recoverable byrecover())go test -raceThis is a real-world issue. For example, when testing code that passes
*sql.DBto mocked methods, the database connection cleaner goroutine modifies internal fields while testify formats the struct for diff output.Solution
Introduces
safeFormatArg()which uses%p(address only) for reference types (Ptr,Map,Slice,Chan) instead of%v, avoiding traversal of the underlying data structure entirely. All other types (structs, primitives, strings) use the full%vrepresentation since they are value types and cannot race.Why not deep-copy?
Copying the arguments before formatting seems like it would preserve the full
%voutput, but:concurrent map iteration and map writeerror-raceduring the copy itselfWhy not recover()?
concurrent map iteration and map writeis a runtime throw, not a panic —recover()cannot catch it-racedetector flags the concurrent access before any panic occurs, causing test failuresTests
Three new concurrent-modification tests:
Test_Arguments_Diff_ConcurrentPointerModification— pointer struct modified by another goroutine (reproducer from mock: Diff is prone to data races #1597)Test_Arguments_Diff_ConcurrentMapModification— map modified by another goroutineTest_Arguments_Diff_ConcurrentSliceModification— slice modified by another goroutineAll three crash or trigger race detector failures without the fix and pass cleanly with it.
Tradeoff
Reference-type arguments now show addresses instead of values in diff output (e.g.,
([]int=0xc0000b4030)instead of([]int=[1 2 3])). The detailed structural diff (fromObjectsAreEqual) still appears above the summary line when arguments don't match, so the diagnostic value loss is minimal.Fixes #1597