mock: Diff is prone to data races #1597
Comments
dolmen
changed the title
|
You can also trigger this issue with this simple code below using 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. |
|
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 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 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 For now, we generally revert to writing a Mock by hand in that case, which is a pity and a pain 😅 |
|
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 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 |
|
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 Thanks for the workaround BTW! |
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 -racethat originate from testify code. More specifically these data races occur whenever a mock pointer argument is concurrently modified. The reason being thatArguments.Diffuses the%vformat 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:
The text was updated successfully, but these errors were encountered: