Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

deadlock on unexpected call with mocked argument implementing fmt.Stringer #116

Closed
jpicht opened this issue Nov 3, 2023 · 1 comment · Fixed by #144
Closed

deadlock on unexpected call with mocked argument implementing fmt.Stringer #116

jpicht opened this issue Nov 3, 2023 · 1 comment · Fixed by #144

Comments

@jpicht
Copy link

jpicht commented Nov 3, 2023

Actual behavior Deadlock on unexpected call to function where one or more arguments are a generated mock that implements fmt.Stringer.

Expected behavior No deadlock on an unexpected call

To Reproduce Steps to reproduce the behavior

  1. put the files below into a directory
  2. mockgen -package=deadlock -source=interfaces.go -destination=mocks.go
  3. go test -v .

interfaces.go

package deadlock

import "fmt"

type Outer interface {
	Consume(Inner)
}

type Inner interface {
	fmt.Stringer
}

deadlock_test.go

package deadlock

import (
	"testing"

	"go.uber.org/mock/gomock"
)

func TestDeadlock(t *testing.T) {
	ctrl := gomock.NewController(t)
	inner := NewMockInner(ctrl)
	outer := NewMockOuter(ctrl)
	outer.Consume(inner)
}

Additional Information

  • gomock mode (reflect or source): source
  • gomock version or git ref: v0.3.0
  • golang version: go version go1.21.3 linux/amd64

Triage Notes for the Maintainers

  1. The call to ctrl.T.Fatalf in controller.go:209 is holding ctrl.mu
  2. Fatalf will call the String() method on the mock (also unexpectedly)
  3. this will reach controller.go:200 and deadlock

backtrace

goroutine 18 [sync.Mutex.Lock]:
sync.runtime_SemacquireMutex(0x0?, 0x65?, 0x4be6c0?)
        /usr/local/go/src/runtime/sema.go:77 +0x25
sync.(*Mutex).lockSlow(0xc000196400)
        /usr/local/go/src/sync/mutex.go:171 +0x15d
sync.(*Mutex).Lock(...)
        /usr/local/go/src/sync/mutex.go:90
go.uber.org/mock/gomock.(*Controller).Call.func1(0xc0001963f0, {0x517220?, 0xc000184200}, {0x536f30, 0x6}, {0x0, 0x0, 0x0})
        .../go/pkg/mod/go.uber.org/[email protected]/gomock/controller.go:200 +0xd1                    
go.uber.org/mock/gomock.(*Controller).Call(0xc0001963f0, {0x517220, 0xc000184200}, {0x536f30, 0x6}, {0x0, 0x0, 0x0})
        .../go/pkg/mod/go.uber.org/[email protected]/gomock/controller.go:225 +0xa7                    
.../deadlock.(*MockInner).String(0xc000184200)                        
        /home/.../deadlock/mocks.go:78 +0x53                                  
fmt.(*pp).handleMethods(0xc00019a680, 0x184220?)
        /usr/local/go/src/fmt/print.go:673 +0x2b2
fmt.(*pp).printValue(0xc00019a680, {0x513ea0?, 0xc000184220?, 0x49e866?}, 0x76, 0x1)
        /usr/local/go/src/fmt/print.go:770 +0xca
fmt.(*pp).printValue(0xc00019a680, {0x50dd20?, 0xc0001a8090?, 0x5398db?}, 0x76, 0x0)
        /usr/local/go/src/fmt/print.go:912 +0x1605
fmt.(*pp).printArg(0xc00019a680, {0x50dd20?, 0xc0001a8090}, 0x76)
        /usr/local/go/src/fmt/print.go:759 +0x6a5
fmt.(*pp).doPrintf(0xc00019a680, {0x540a4e, 0x2e}, {0xc000180140?, 0x5, 0x5})
        /usr/local/go/src/fmt/print.go:1077 +0x39e
fmt.Sprintf({0x540a4e, 0x2e}, {0xc000180140, 0x5, 0x5})
        /usr/local/go/src/fmt/print.go:239 +0x53
testing.(*common).Fatalf(0xc00019e680, {0x540a4e?, 0x40ac65?}, {0xc000180140?, 0x50efa0?, 0x61ef01?})
        /usr/local/go/src/testing/testing.go:1082 +0x3f
go.uber.org/mock/gomock.(*Controller).Call.func1(0xc0001963f0, {0x5172a0?, 0xc000184210}, {0x537171, 0x7}, {0xc000184220, 0x1, 0x1})
        .../go/pkg/mod/go.uber.org/[email protected]/gomock/controller.go:209 +0x356                    
go.uber.org/mock/gomock.(*Controller).Call(0xc0001963f0, {0x5172a0, 0xc000184210}, {0x537171, 0x7}, {0xc000184220, 0x1, 0x1})
        .../go/pkg/mod/go.uber.org/[email protected]/gomock/controller.go:225 +0xa7                    
.../deadlock.(*MockOuter).Consume(0xc000184210, {0x5670e0?, 0xc000184200})                        
        /home/.../deadlock/mocks.go:43 +0xab                                  
.../deadlock.TestDeadlock(0x0?)                        
        /home/.../deadlock_test.go:13 +0x129                                           
testing.tRunner(0xc00019e680, 0x542e38)
        /usr/local/go/src/testing/testing.go:1595 +0xff
created by testing.(*T).Run in goroutine 1
        /usr/local/go/src/testing/testing.go:1648 +0x3ad
FAIL    .../deadlock    1.006s
FAIL
nbgraham added a commit to nbgraham/mock that referenced this issue Jan 25, 2024
@nbgraham
fix: stringer deadlock uber-go#116
2f9e900
@nbgraham nbgraham mentioned this issue Jan 25, 2024
Merged
JacobOaks pushed a commit that referenced this issue Feb 6, 2024
@nbgraham
fix: prevent stringer deadlock #116 (#144)
6d8e954 
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).
@eyasy1217 eyasy1217 mentioned this issue Jul 10, 2024
Closed
@jpicht
Copy link
Author

jpicht commented Jul 17, 2024 •
edited
Loading

This is not fixed in main. Can you please reopen this?
Ignore this - I missed to regen one file. Sorry for the noise.

JacobOaks added a commit to JacobOaks/mock that referenced this issue Sep 18, 2024
@JacobOaks
Avoid stringer-related deadlocks without adding ISGOMOCK
768f9ed 
A deadlock related to controller calling Stringer on mocks themselves
was revealed in uber-go#116. uber-go#114 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.

Ref: uber-go#116
JacobOaks added a commit to JacobOaks/mock that referenced this issue Sep 18, 2024
@JacobOaks
Avoid stringer-related deadlocks without adding ISGOMOCK
c0bb3f2 
A deadlock related to controller calling Stringer on mocks themselves
was revealed in uber-go#116. uber-go#114 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.

Ref: uber-go#116
JacobOaks added a commit to JacobOaks/mock that referenced this issue Sep 18, 2024
@JacobOaks
Avoid stringer-related deadlocks without adding ISGOMOCK
9a164ea 
A deadlock related to controller calling Stringer on mocks themselves
was revealed in uber-go#116. uber-go#114 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`.

Ref: uber-go#116
@JacobOaks JacobOaks mentioned this issue Sep 18, 2024
Merged
JacobOaks added a commit to JacobOaks/mock that referenced this issue Sep 18, 2024
@JacobOaks
Avoid stringer-related deadlocks without adding ISGOMOCK
f042bfa 
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
JacobOaks added a commit to JacobOaks/mock that referenced this issue Sep 18, 2024
@JacobOaks
Avoid stringer-related deadlocks without adding ISGOMOCK
1f58549 
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.

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

Ref: uber-go#193
JacobOaks added a commit that referenced this issue Sep 18, 2024
@JacobOaks
Avoid stringer-related deadlocks without adding ISGOMOCK (#204)
33ccc99 
A deadlock related to controller calling Stringer on mocks themselves
was revealed in #116. #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.

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

Ref: #193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: prevent stringer deadlock #116 nbgraham/mock
1 participant
@jpicht