Skip to content

Commit

Permalink
fix: prevent stringer deadlock #116 (#144)
Browse files Browse the repository at this point in the history
Fixes #116 

# Problem

If your mock an interface that matches the fmt.Stringer interface:

```go
type Stringer interface {
  String() string
}
```

Then your unit tests can deadlock if you provide that mock as an
expected argument to a call that is not matched (i.e. fails the test).

Because, when printing the error message for the call that was not
matched, it calls `String()` on all arguments that support it, including
the mock.

But each call to a mock is protected with a mutex, and the previous call
(that was not matched) has not yet exited.

# Solution
The solution has two parts

1. During mock code generation (an existing part of this library) add a
unique method on mocks (ISGOMOCK)
1. During test execution, whenever we are stringifying something that
might be a mock, check if it is a mock (ISGOMOCK) that implements the
String() method. If it is, just use the type name as the string value,
instead of calling String() (which would cause the deadlock).
  • Loading branch information
nbgraham authored Feb 6, 2024
1 parent aaa9d14 commit 6d8e954
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 2 deletions.
6 changes: 5 additions & 1 deletion gomock/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,11 @@ func (ctrl *Controller) Call(receiver any, method string, args ...any) []any {
// and this line changes, i.e. this code is wrapped in another anonymous function.
// 0 is us, 1 is controller.Call(), 2 is the generated mock, and 3 is the user's test.
origin := callerInfo(3)
ctrl.T.Fatalf("Unexpected call to %T.%v(%v) at %s because: %s", receiver, method, args, origin, err)
stringArgs := make([]string, len(args))
for i, arg := range args {
stringArgs[i] = getString(arg)
}
ctrl.T.Fatalf("Unexpected call to %T.%v(%v) at %s because: %s", receiver, method, stringArgs, origin, err)
}

// Two things happen here:
Expand Down
16 changes: 16 additions & 0 deletions gomock/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,22 @@ func TestNoRecordedMatchingMethodNameForAReceiver(t *testing.T) {
})
}

func TestNoStringerDeadlockOnError(t *testing.T) {
reporter, ctrl := createFixtures(t)
subject := new(Subject)
mockFoo := NewMockFoo(ctrl)
var _ fmt.Stringer = mockFoo

ctrl.RecordCall(subject, "FooMethod", mockFoo)
reporter.assertFatal(func() {
ctrl.Call(subject, "NotRecordedMethod", mockFoo)
}, "Unexpected call to", "there are no expected calls of the method \"NotRecordedMethod\" for that receiver")
reporter.assertFatal(func() {
// The expected call wasn't made.
ctrl.Finish()
})
}

// This tests that a call with an arguments of some primitive type matches a recorded call.
func TestExpectedMethodCall(t *testing.T) {
reporter, ctrl := createFixtures(t)
Expand Down
2 changes: 1 addition & 1 deletion gomock/matchers.go
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@ func (e eqMatcher) Matches(x any) bool {
}

func (e eqMatcher) String() string {
return fmt.Sprintf("is equal to %v (%T)", e.x, e.x)
return fmt.Sprintf("is equal to %s (%T)", getString(e.x), e.x)
}

type nilMatcher struct{}
Expand Down
19 changes: 19 additions & 0 deletions gomock/mock_test.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

25 changes: 25 additions & 0 deletions gomock/string.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package gomock

import "fmt"

type mockInstance interface {
ISGOMOCK() struct{}
}
type mockedStringer interface {
fmt.Stringer
mockInstance
}

// getString is a safe way to convert a value to a string for printing results
// If the value is a a mock, getString avoids calling the mocked String() method,
// which avoids potential deadlocks
func getString(x any) string {
switch v := x.(type) {
case mockedStringer:
return fmt.Sprintf("%T", v)
case fmt.Stringer:
return v.String()
default:
return fmt.Sprintf("%v", v)
}
}
8 changes: 8 additions & 0 deletions mockgen/mockgen.go
Original file line number Diff line number Diff line change
Expand Up @@ -502,6 +502,14 @@ func (g *generator) GenerateMockInterface(intf *model.Interface, outputPackagePa
g.out()
g.p("}")

// XXX: possible name collision here if someone has ISGOMOCK in their interface.
g.p("// ISGOMOCK indicates that this struct is a gomock mock.")
g.p("func (m *%v%v) ISGOMOCK() struct{} {", mockType, shortTp)
g.in()
g.p("return struct{}{}")
g.out()
g.p("}")

g.GenerateMockMethods(mockType, intf, outputPackagePath, longTp, shortTp, *typed)

return nil
Expand Down

0 comments on commit 6d8e954

Please sign in to comment.