mock: Diff is prone to data races

[petergardfjall]

Description

Testify mocks are currently prone to data races when supplied a pointer argument (that may be concurrently updated elsewhere).

In several instances of our code, we have started seeing data races when running go test -race that originate from testify code. More specifically these data races occur whenever a mock pointer argument is 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.

We did not see these issues earlier, but they started appearing lately after we upgraded to go 1.22. Not sure if that is a coincidence, but maybe performance improved in such a way that these data races suddenly started surfacing.

We need to test our code for race conditions with go test -race, and we cannot have testify be the offender of data races.

A PR to resolve the issue is suggested in #1598.
(It should be noted that it is heavily inspired by #1229, but since that PR hasn't seen any updates in over a year I decided to start fresh).

Step To Reproduce

See test case in suggested fix PR: #1598

Expected behavior

The unit test in #1598 passes when running with go test -race.

Actual behavior

The unit test fails with a data race:

$ go test -race ./...
ok  	github.com/stretchr/testify	(cached)
ok  	github.com/stretchr/testify/assert	(cached)
ok  	github.com/stretchr/testify/assert/internal/unsafetests	(cached)
?   	github.com/stretchr/testify/http	[no test files]
==================
WARNING: DATA RACE
Write at 0x00c000591f30 by goroutine 127:
  github.com/stretchr/testify/mock.Test_CallMockWithConcurrentlyModifiedPointerArg.func1()
      /home/peterg/dev/git/testify/mock/mock_test.go:1938 +0x84

Previous read at 0x00c000591f30 by goroutine 126:
  reflect.typedmemmove()
      /opt/go/src/runtime/mbarrier.go:203 +0x0
  reflect.packEface()
      /opt/go/src/reflect/value.go:135 +0xc5
  reflect.valueInterface()
      /opt/go/src/reflect/value.go:1526 +0x179
  reflect.Value.Interface()
      /opt/go/src/reflect/value.go:1496 +0xb4
  fmt.(*pp).printValue()
      /opt/go/src/fmt/print.go:769 +0xc5
  fmt.(*pp).printValue()
      /opt/go/src/fmt/print.go:921 +0x132a
  fmt.(*pp).printArg()
      /opt/go/src/fmt/print.go:759 +0xb84
  fmt.(*pp).doPrintf()
      /opt/go/src/fmt/print.go:1174 +0x10ce
  fmt.Sprintf()
      /opt/go/src/fmt/print.go:239 +0x5c
  github.com/stretchr/testify/mock.Arguments.Diff()
      /home/peterg/dev/git/testify/mock/mock.go:950 +0x1b2
  github.com/stretchr/testify/mock.(*Mock).findExpectedCall()
      /home/peterg/dev/git/testify/mock/mock.go:368 +0x147
  github.com/stretchr/testify/mock.(*Mock).MethodCalled()
      /home/peterg/dev/git/testify/mock/mock.go:476 +0xac
  github.com/stretchr/testify/mock.(*Mock).Called()
      /home/peterg/dev/git/testify/mock/mock.go:466 +0x195
  github.com/stretchr/testify/mock.(*pointerArgMock).Question()
      /home/peterg/dev/git/testify/mock/mock_test.go:1919 +0x8c
  github.com/stretchr/testify/mock.Test_CallMockWithConcurrentlyModifiedPointerArg()
      /home/peterg/dev/git/testify/mock/mock_test.go:1943 +0x28b
  testing.tRunner()
      /opt/go/src/testing/testing.go:1689 +0x21e
  testing.(*T).Run.gowrap1()
      /opt/go/src/testing/testing.go:1742 +0x44

Goroutine 127 (running) created at:
  github.com/stretchr/testify/mock.Test_CallMockWithConcurrentlyModifiedPointerArg()
      /home/peterg/dev/git/testify/mock/mock_test.go:1936 +0x27c
  testing.tRunner()
      /opt/go/src/testing/testing.go:1689 +0x21e
  testing.(*T).Run.gowrap1()
      /opt/go/src/testing/testing.go:1742 +0x44

Goroutine 126 (running) created at:
  testing.(*T).Run()
      /opt/go/src/testing/testing.go:1742 +0x825
  testing.runTests.func1()
      /opt/go/src/testing/testing.go:2161 +0x85
  testing.tRunner()
      /opt/go/src/testing/testing.go:1689 +0x21e
  testing.runTests()
      /opt/go/src/testing/testing.go:2159 +0x8be
  testing.(*M).Run()
      /opt/go/src/testing/testing.go:2027 +0xf17
  main.main()
      _testmain.go:267 +0x2bd
==================
--- FAIL: Test_CallMockWithConcurrentlyModifiedPointerArg (0.00s)
    testing.go:1398: race detected during execution of test
FAIL
FAIL	github.com/stretchr/testify/mock	0.053s
ok  	github.com/stretchr/testify/require	(cached)
ok  	github.com/stretchr/testify/suite	(cached)
FAIL

You can also trigger this issue with this simple code below using io.Pipe, with Go 1.22.5. No problem with Go 1.21.12.

package foo

import (
	"io"
	"testing"

	"github.com/stretchr/testify/mock"
)

type Mock struct {
	mock.Mock
}

func (m *Mock) Read(r io.Reader) {
	m.Called(r)
}

func TestDataRace(t *testing.T) {
	var m Mock
	m.On("Read", mock.Anything)

	r, w := io.Pipe()

	go func() {
		_ = w.Close()
	}()

	m.Read(r) // trigger m.Called(r) in Mock::Read() --> DATA RACE

	_ = r.Close()
}

The fix proposed in #1598 fixes also this issue in this usecase.

[RouquinBlanc]

This is also an issue we have each time we want to test things passing references to complex structs with private data protected by locks.

Consider the following example:

type Connection interface {
    Write([]byte) error
    Close() error
}

type EventHandler interface {
    HandleEvent(connection Connection, data []byte)
}

This issue triggers race checks frequently; the actual struct implementing Connection does change during the processing of code, but it is something normal, and the changes are protected by a mutex or other synchronization.

When doing something like this (using mockery):

myConnection := NewRealConnection() // concrete struct pointer implementing Connection

eventHandler = NewMockEventHandler(t)

// case1: expect the same connection
eventHandler.EXPECT().HandleEvent(myConnection, mock.Anything) 

// case2: expect whatever
eventHandler.EXPECT().HandleEvent(mock.Anything, mock.Anything) 

The expectation for case1 is that the pointer passed during the call matches myConnection pointer (no assumptions on the internal state), and in case2 we explicitly do not care what is passed at all, just that it is called.

The #1598 PR fixes the problem for us; in more general terms, my expectation on pointers is that the pointer matches, not that the underlying data is identical, which is practically impossible to have on a mutating object, whose internal fields should be accessed under lock and are irrelevant at the EXPECT level.

Or, if you fear this would lead to regressions, to have a way to explicitly say "I just care about the pointer" (for case1) like mock.Pointer(myConnection) or "I really don't care" (for case2) with some kind of mock.AnyPointer or something similar?

For now, we generally revert to writing a Mock by hand in that case, which is a pity and a pain 😅

[brackendawson]

I am happy with #1598's approach to fixing this by printing just address of pointer types when the expectation is not matched.

Issue at the moment @RouquinBlanc is that the PR fixes a subset of types this can happen to, which does include your example. But if the implementation of Connection was a non-pointer but contained pointer fields protected by mutexes, then the race comes right back.

It would be great if go-spew had an option to not follow pointers but it doesn't, and I don't think we'll get it added.

I think our options are:
a. Never show any detail about non-matching args. I expect that this will upset a lot of people.
b. Write a formatter to show only the safely accessible parts of the structure.

A workaround you can use now that isn't writing a custom mock, is to implement the stringer interface safely on your type:

type MyStruct{
	mux sync.Mutex
	criticalNumber int
}

func (m *MyStruct) String() string {
	m.mux.Lock()
	defer m.mux.Unlock()
	return fmt.Sprintf("%#v", m)
}

And then pointers to MyStruct can safely be passed to mock.On and mock.Called. Or you can even just return empty string.

[RouquinBlanc]

Hi @brackendawson, indeed the problem remains when comparing a non-pointer structure.

What I was trying to defend is the fact that if I ask my mock to catch calls with mock.Anything, I don't expect any check at all to be done about expectations on the arguments, even less reading internal fields. And in cases where you want to only validate that the call is made with a particular pointer (without a comparison of what's behind that top-level pointer), to have a way to express that.

Thanks for the workaround BTW!

[peczenyj]

I have the same issue, at some point I have a method that receives *sql.Tx and even with mock.Anything I got the race condition if I run the tests with -race and I tested the pull request #1693 - it works fine

WARNING: DATA RACE
Read at 0x00c00033b4c0 by goroutine 22:
  reflect.typedmemmove()
      /usr/local/go1.23.5/src/runtime/mbarrier.go:225 +0x0
  reflect.packEface()
      /usr/local/go1.23.5/src/reflect/value.go:133 +0xc5
  reflect.valueInterface()
      /usr/local/go1.23.5/src/reflect/value.go:1510 +0x179
  reflect.Value.Interface()
      /usr/local/go1.23.5/src/reflect/value.go:1481 +0xb4
  fmt.(*pp).printValue()
      /usr/local/go1.23.5/src/fmt/print.go:769 +0xc5
  fmt.(*pp).printValue()
      /usr/local/go1.23.5/src/fmt/print.go:921 +0x130a
  fmt.(*pp).printArg()
      /usr/local/go1.23.5/src/fmt/print.go:759 +0xb84
  fmt.(*pp).doPrintf()
      /usr/local/go1.23.5/src/fmt/print.go:1173 +0x10ce
  fmt.Sprintf()
      /usr/local/go1.23.5/src/fmt/print.go:239 +0x5c
  github.com/stretchr/testify/mock.Arguments.Diff()
...
Previous write at 0x00c00033b4c0 by goroutine 25:
  sync/atomic.CompareAndSwapInt32()
      /usr/local/go1.23.5/src/runtime/race_amd64.s:361 +0xb
  sync/atomic.CompareAndSwapUint32()
      <autogenerated>:1 +0x18
  database/sql.(*Tx).rollback()
      /usr/local/go1.23.5/src/database/sql/sql.go:2319 +0x4a
  database/sql.(*Tx).awaitDone()
      /usr/local/go1.23.5/src/database/sql/sql.go:2213 +0x6b
  database/sql.(*DB).beginDC.gowrap1()
      /usr/local/go1.23.5/src/database/sql/sql.go:1917 +0x33

I spent like hours trying to find the race condition on my code, then I realize it only happens in the mock library :/

Unfortunately, I cant substitute the *sql.Tx object by some other implementation (like to add a safe stringer)

[peczenyj]

Hello

I have an workaround: for each mock type I need to create a wrapped type that include the original mock and override the affected methods, calling the mock with a safe value (zero value) to avoid the race condition. I did not like this approach but I have no other option since the last comment here was done 2 weeks ago and I don't want to do use a fork of this library