Open oceanful opened 3 years ago
Hi @oceanful, thanks for opening this issue. It's true, Pegomock doesn't allow mock-setup in multiple goroutines (just for clarity, invoking the mocked methods from multiple goroutines is not a problem).
Introducing a Mutex for this is actually not quite that trivial. As you noted, we'd have to lock the slice of matchers that a certain goroutine used for mock-setup. In this world, any call to a matcher like StringEq
would first lock the mutex, any call to When
would release it as a last step. I think it's possible, but I'm not sure yet, if that's the best way to go, due to the added complexity. I'm also not entirely convinced yet that a mocking framework should be thread-safe for its setup and verification methods, but I'm happy to hear arguments here :-).
2 suggestions that I'd like to go through first, before diving into the suggested Mutex:
Hi Peter, thank you for the thoughtful answer!
I see now that because of the way the dsl is structured, the When()
and matcher methods have to coordinate with mock invocation, and each other, via global variables lastInvocation
and globalArgMatchers
, which is indeed tricky.
One idea would be an (optional) alternative grammar that allows the framework to grab a lock before the matchers and invocation are called ... something like:
func WhenInvoking() func(invocation ...interface{}) *ongoingStubbing {
invocationLock.Lock()
return func(invocation ...interface{}) *ongoingStubbing {
defer invocationLock.Unlock()
return When(invocation...)
}
}
Which would make testing code read like:
WhenInvoking()(myMock.Method(AnyString(), AnyInt())).Then(...)
Not quite as pretty as the current DSL but perhaps not too bad of an alternative when parallelism is desired. I think this can be done without breaking compatibility, but I might not have thought everything through.
Also, thank you for offering alternatives!
External locking is exactly how we're currently working around this issue, but it's obviously not ideal to add this manual coordination in each testing package.
We did look at Ginkgo a while ago and decided that it was too heavyweight at the time, but we may revisit this.
Hey @oceanful, I think your approach could work. But agree that introducing WhenInvoking
wouldn't be quite as pretty. I'm wondering, if one could not achieve a similar logic as suggested by you by exploiting the fact that invocation ...interface{}
is not really used. It's only used if it's a function, i.e. that function actually gets called. Would there be a way to use that to run things in a confined scope and introduce lock/unlock logic? Just thinking out loud here.
Hi Peter,
If we want to fix this in the current DSL, then I agree that your initial proposal is the way to go; grab the lock if not already held in RegisterMatcher()
, When()
, and Verify()
, and release it in the latter two in a defer statement.
func RegisterMatcher(...) {
lockIfNotLocked()
...
}
func When(...) {
lockIfNotLocked()
defer unlock()
...
}
func Verify(...) {
lockIfNotLocked()
defer unlock()
...
}
As you've probably surmised, the When()
and Verify()
do need to grab the lock so that a goroutine that's working on an invocation without matchers does not interfere with a goroutine that's working on an invocation with matchers. But I do believe your proposal will effectively make it thread-safe.
When running tests in
t.Parallel()
, the Golang race detector often fails with a data race:Perhaps this can be addressed by locking on a Mutex when accessing
globalArgMatchers
, or encapsulating the slice of Matchers in a thread-safe object.Or let me know if there's a different approach I should be taking for using mocks in a parallel testing environment. Thank you!