Skip to content

Commit

Permalink
Avoid stringer-related deadlocks without adding ISGOMOCK
Browse files Browse the repository at this point in the history
A deadlock related to controller calling Stringer on mocks themselves
was revealed in uber-go#116. uber-go#144 solved this deadlock by adding a generated
`ISGOMOCK()` method to all generated mocks, and then checking for it
before calling `.String()` on arguments.

This reveals an exported method on each generated mock that:
* Bloats the generated code
* Can be taken dependency on in strange ways via Hyrum's Law
* Technically opens up another route for naming collision.

This PR attempts to clean up this type of check by instead generating
an unexported field in generated mock structs instead, and checks for it using reflect.
This hides this implementation detail of gomock/mockgen better,
and produces less generated bloat.

This PR then regenerated all generated code for tests/example via `go generate`.

Note that, importantly, the regression test added in uber-go#144 still passes
with this PR.

Ref: uber-go#193
  • Loading branch information
JacobOaks committed Sep 18, 2024
1 parent 60372e3 commit f042bfa
Show file tree
Hide file tree
Showing 34 changed files with 61 additions and 189 deletions.
6 changes: 1 addition & 5 deletions gomock/internal/mock_gomock/mock_matcher.go

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

6 changes: 1 addition & 5 deletions gomock/mock_test.go

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

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

import "fmt"

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

// 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)
if isGeneratedMock(x) {
return fmt.Sprintf("%T", x)
}
if s, ok := x.(fmt.Stringer); ok {
return s.String()
}
return fmt.Sprintf("%v", x)
}

// isGeneratedMock checks if the given type has a "isgomock" field,
// indicating it is a generated mock.
func isGeneratedMock(x any) bool {
typ := reflect.TypeOf(x)
if typ == nil {
return false
}
if typ.Kind() == reflect.Ptr {
typ = typ.Elem()
}
if typ.Kind() != reflect.Struct {
return false
}
_, isgomock := typ.FieldByName("isgomock")
return isgomock
}
6 changes: 1 addition & 5 deletions mockgen/internal/tests/add_generate_directive/mock.go

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

6 changes: 1 addition & 5 deletions mockgen/internal/tests/const_array_length/mock.go

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

6 changes: 1 addition & 5 deletions mockgen/internal/tests/copyright_file/mock.go

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

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

6 changes: 1 addition & 5 deletions mockgen/internal/tests/defined_import_local_name/mock.go

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

6 changes: 1 addition & 5 deletions mockgen/internal/tests/dot_imports/mock.go

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

6 changes: 1 addition & 5 deletions mockgen/internal/tests/empty_interface/mock.go

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

6 changes: 1 addition & 5 deletions mockgen/internal/tests/exclude/mock.go

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

6 changes: 1 addition & 5 deletions mockgen/internal/tests/extra_import/mock.go

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

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

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

6 changes: 1 addition & 5 deletions mockgen/internal/tests/import_embedded_interface/net_mock.go

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

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

6 changes: 1 addition & 5 deletions mockgen/internal/tests/import_source/source_mock.go

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

Loading

0 comments on commit f042bfa

Please sign in to comment.