Global matcher registration is not thread-safe

[oceanful]

When running tests in t.Parallel(), the Golang race detector often fails with a data race:

==================
WARNING: DATA RACE
Read at 0x000003717960 by goroutine 48:
  github.com/petergtz/pegomock.(*Matchers).append()
      /Users/ocean/go/pkg/mod/github.com/petergtz/[email protected]+incompatible/dsl.go:385 +0x143
  github.com/petergtz/pegomock.RegisterMatcher()
      /Users/ocean/go/pkg/mod/github.com/petergtz/[email protected]+incompatible/dsl.go:47 +0x133
      ...

Previous write at 0x000003717960 by goroutine 52:
  github.com/petergtz/pegomock.(*GenericMock).Verify.func1()
      /Users/ocean/go/pkg/mod/github.com/petergtz/[email protected]+incompatible/dsl.go:114 +0x3a
  github.com/petergtz/pegomock.(*GenericMock).Verify()
      /Users/ocean/go/pkg/mod/github.com/petergtz/[email protected]+incompatible/dsl.go:157 +0x714
      ...

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!

[petergtz]

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:

1. Can you put your mock-setup calls into a shared Mutex so you can handle the locking on a broader level? Depending on your test setup that might not be too complex.
2. Have you tried Ginkgo? Ginkgo allows running tests in parallel, but runs the test in separate processes to avoid all kinds of race conditions. I think Ginkgo's way of running tests in parallel is better, because it leaves it up to the test framework what/when to run in parallel and you can change this during test-run time, not implementation time.

[oceanful]

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!

1. 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.

2. We did look at Ginkgo a while ago and decided that it was too heavyweight at the time, but we may revisit this.

[petergtz]

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.

[oceanful]

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.