Skip to content

mock: fix data race in Arguments.Diff for pointer-like arguments#1865

Closed
pavelzeman wants to merge 1 commit intostretchr:masterfrom
pavelzeman:fix/arguments-diff-race
Closed

mock: fix data race in Arguments.Diff for pointer-like arguments#1865
pavelzeman wants to merge 1 commit intostretchr:masterfrom
pavelzeman:fix/arguments-diff-race

Conversation

@pavelzeman
Copy link
Copy Markdown

Summary

Fixes Arguments.Diff causing data races when mock arguments (pointers, maps, slices) are concurrently modified by other goroutines.

Problem

Arguments.Diff uses fmt.Sprintf("%v") to format arguments for diff output. This traverses the underlying data structure, which races with concurrent writers:

  • Maps: triggers fatal error: concurrent map iteration and map write — crashes the entire test process (non-recoverable by recover())
  • Pointers/slices: causes data races detectable by go test -race

This is a real-world issue. For example, Mattermost hit this with *sql.DB arguments — the database connection cleaner goroutine modifies internal fields while testify formats the struct for diff output.

Solution

Introduces safeFormatArg() with targeted handling per kind:

Kind Strategy Rationale
Map %p (address only) Map iteration is fatally unsafe with concurrent writes — no recovery possible
Ptr, Slice %v with panic recovery → %p fallback Races produce recoverable panics; preserves full output when safe
Everything else %v (unchanged) Value types cannot race

This preserves the full %v diff output for the common case (non-concurrent arguments, value types, pointers/slices without active races) while preventing testify from being the source of data races or fatal crashes.

Tests

Three new race-condition tests:

  1. Test_Arguments_Diff_ConcurrentPointerModification — adapted from mock: avoid data races in Arguments.Diff #1598, pointer struct modified by another goroutine
  2. Test_Arguments_Diff_ConcurrentMapModification — raised by @brackendawson in mock: avoid data races in Arguments.Diff #1598 review, map modified by another goroutine
  3. Test_Arguments_Diff_ConcurrentSliceModification — slice modified by another goroutine

All three crash/race without the fix and pass cleanly with it.

Alternatives Considered

  • mock: avoid data races in Arguments.Diff #1598: Uses %p for all reflect.Ptr kinds. Doesn't cover maps (fatal) or slices. Loses diff output even when arguments aren't being concurrently modified.
  • Deep-copy at capture time: copyArgument() itself races for maps (iterating to copy triggers the same fatal error). Would require external synchronization that testify can't enforce.
  • Deep-copy in MethodCalled: Same map iteration problem, plus significant performance overhead for every mock call.

Related

Fixes #1597
Related to #1598 (alternative approach, open since 2024)

@pavelzeman pavelzeman force-pushed the fix/arguments-diff-race branch from b0cfab8 to 47e2cd4 Compare April 3, 2026 02:59
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 handles each case:
- Maps: uses %p (address-only) since map iteration cannot be made safe
  without external synchronization
- Pointers/slices: attempts %v with panic recovery, falls back to %p
- All other types: unchanged %v behavior (value types, no race possible)

This preserves the full %v diff output for the common case (non-concurrent
arguments, value types, pointers/slices without races) while preventing
testify itself from being the source of data races in user tests.

Fixes stretchr#1597
Related: stretchr#1598 (alternative approach using %p for all pointers)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

mock: Diff is prone to data races

1 participant