fixing panic in calls to assertion with nil m.mutex #1212
Conversation
This reverts a change that was made in stretchr#1182 The PR makes m.mutex a pointer which now needs to be checked but it's not checked for nil everywhere. This should also help with these issues: - stretchr#1208 - stretchr#1210
…dy have it locked
|
Hey @boyan-soubachov sorry to ping you directly but since you were the original author for #1182 I thought I'd bring this one to your attention. Thanks for any review you might be able to offer. |
| } | ||
| for _, obj := range testObjects { | ||
| if m, ok := obj.(Mock); ok { | ||
| if m, ok := obj.(*Mock); ok { |
There was a problem hiding this comment.
Are you sure this should be changed? IIRC I didn't change this logic in the PR I merged?
There was a problem hiding this comment.
Yes, because we want to refer to the same instance of sync.Mutex for the copy that is a type check here.
There was a problem hiding this comment.
Also, go vet ./... will show that warning for us ( It was caught in CI).
There was a problem hiding this comment.
Fair enough; I see it's a test helper anyways.
Thank you for catching this!
There was a problem hiding this comment.
🤦 . I must've missed this when doing the Go 1.18 PR (which broke this) and 'fixed it' the long way around. Thanks
There was a problem hiding this comment.
This was in fact incorrect. It makes the entire if statement into a no-op and introduced a new panic in #1227
Previously passing a mock.Mock by value rather than by reference worked (in cases without mutex locking issues) and logged a warning. This was broken by stretchr#1212 which introduced a breaking change in an attempt to fix go vet. There is no clean way to fix the breaking change as we now have (and want) go vet in our CI. This PR does not revert the breaking change but changes the panic to a test failure with a useful message.
Previously passing a mock.Mock by value rather than by reference worked (in cases without mutex locking issues) and logged a warning. This was broken by stretchr#1212 which introduced a breaking change in an attempt to fix go vet. There is no clean way to fix the breaking change as we now have (and want) go vet in our CI. This PR does not revert the breaking change but changes the panic to a test failure with a useful message.
3 participants
Summary
@CleanCut and I found these changes that should be reverted as they are causing panics after library updates.
This reverts a change that was made in #1182
That PR makes m.mutex a pointer which now needs to be checked but it's not checked for nil everywhere.
It also reset the mutex in the
.Onfunction when it could actually have locks from other callers.Changes
m.mutexback to a non pointer value.OnfunctionMotivation
Fixing panics and crashes in the library after a minor update.
Related issues
This should also help with these issues: