Proposal: Provide concurrency-safe setter methods for function fields

[sapuri]

There are cases where we want to dynamically change the behavior of a mock method during testing. By default, moq generates a field like MyMethodFunc func(...) for each interface method, and we can reassign the function pointer in our tests. However, this often leads to data races when multiple goroutines are running (especially in parallel tests).

So, how about generating concurrency-safe setter methods (using RWMutex) for each generated function pointer? For example:

//go:generate moq -out generated.go . MyInterface

// MyInterface is a test interface.
type MyInterface interface {
	One() bool
}

After generation, we could have something like:

// ...

func (mock *MyInterfaceMock) SetOneFunc(f func() bool) {
	mock.lockOneFunc.Lock()
	defer mock.lockOneFunc.Unlock()
	mock.OneFunc = f
}

// ...

With this approach, tests can safely change the mock function at runtime without causing data races.

[breml]

Hi @sapuri

I am using moq since many years and I never had a situation, where I wished to dynamically change the behavior of a mock method to the extend, that I wanted to assign a different function while executing the test, especially not in concurrent scenarios. Therefor I ask you to provide more details or even better a concrete example of such a test case for better understanding of the problem.

[sapuri]

@breml Thank you for your response!

Below is an example illustrating why we need to reassign the mock function concurrently and the data race issue that arises. Although the example looks somewhat contrived, similar data races can realistically occur in scenarios where a workflow engine (like Temporal) runs in the background. In such systems, the mock’s method might be called by one goroutine while another goroutine updates the mock’s behavior to simulate changes during runtime.

// myinterface.go
package example

type MyInterface interface {
	One() bool
}

// Suppose moq generates something like:
//
// type MyInterfaceMock struct {
// 	OneFunc func() bool
// }
//
// In the current implementation, there's no concurrency-safe way
// to reassign OneFunc during test execution.

// myinterface_test.go
package example_test

import (
	"sync"
	"testing"
	"time"

	"example"
)

func TestConcurrentMockReassignment(t *testing.T) {
	// created by moq
	mock := &example.MyInterfaceMock{
		OneFunc: func() bool { return false },
	}

	// Goroutine A: continuously calls mock.OneFunc()
	var wg sync.WaitGroup
	wg.Add(1)
	go func() {
		defer wg.Done()
		for i := 0; i < 100000; i++ {
			_ = mock.OneFunc()
			time.Sleep(time.Microsecond)
		}
	}()

	// Goroutine B (main test routine): reassigns OneFunc repeatedly
	// during the same time period as Goroutine A
	for i := 0; i < 100000; i++ {
		mock.OneFunc = func() bool { return true }
		mock.OneFunc = func() bool { return false }
	}

	wg.Wait()
}

As shown, modifying OneFunc while it is being called by another goroutine can lead to data races. Though this example is stripped down, it parallels real-world tests in which a background workflow might invoke mock methods concurrently while the test itself updates the mock’s logic to simulate changing conditions. Providing concurrency-safe setter methods and ensuring moq’s generated code protects both reads and writes with a proper lock would prevent these race conditions.

[breml]

@sapuri Thanks for the example code reproducing the issue. It is absolutely clear, that this code will produce the data race. What is still unclear to me is, what kind of code do you test using moq that requires such changes. Given this code, the results of your tests are no longer deterministic and I wonder, what you assert for in your test logic.

In regards to changing the assignment of OneFunc in you example above, have you considered to have a single implementation of OneFunc, that might return true or false based on some other criteria?
For example, if you need your mocked function to return different return values for each time it is called, you might consider using a queue (see comments #187 (comment) or #160 (comment)). Since the landing of generics in Go, this can also be made in a more generic way. For example with the function in https://github.com/FuturFusion/migration-manager/blob/main/internal/testing/queue/queue.go (used in https://github.com/FuturFusion/migration-manager/blob/main/internal/migration/instance_service_test.go#L124 with values from https://github.com/FuturFusion/migration-manager/blob/main/internal/migration/instance_service_test.go#L30).

As mentioned above, I kind of doubt, that your issue is the way moq behaves. To me, it looks like an issue in how your tests are structured and how you assert for the correct behavior in your tests (but I might be wrong about this, this is why I need to better understand your use case).

[breml]

The examples I provided above for the queue are not concurrency safe, but this would be something, that can be easily added using a mutex.

[sapuri]

@breml Here’s a minimal example that demonstrates my use case:

1. We have MockClock struct with NowFunc field.
2. A WorkflowManager runs workflows which contains a async activity that call clock.Now() in separate goroutines.
3. The test function Prepare() toggles the clock’s NowFunc from one value to another while those background workflows may still be active.

This is conceptually similar to scenarios where a workflow engine like Temporal or custom orchestration logic runs asynchronously, calling a shared mock clock.
Here is the stripped-down code:

// mock_clock.go
package example

import "time"

type MockClock struct {
	NowFunc func() time.Time
}

func (c *MockClock) Now() time.Time {
	return c.NowFunc()
}

// container.go
package example

type Container struct {
	Clock *MockClock
	WorkflowManager *WorkflowManager
}

// interactor.go
package example

import (
	"context"
	"time"
)

// Interactor represents business logic that will trigger workflows.
type Interactor struct {
	c *Container
}

func NewInteractor(c *Container) *Interactor {
	return &Interactor{c: c}
}

func (i *Interactor) CreateSomething(ctx context.Context) error {
	return i.c.WorkflowManager.RunWorkflowA(ctx, i.c.Clock)
}

func (i *Interactor) CaptureSomething(ctx context.Context) error {
	return i.c.WorkflowManager.RunWorkflowB(ctx, i.c.Clock)
}

// workflow.go
package example

import (
	"context"
	"fmt"
	"time"
)

// WorkflowManager simulates a workflow engine that calls Clock.Now() in separate goroutines.
type WorkflowManager struct{}

func (wm *WorkflowManager) RunWorkflowA(ctx context.Context, clock *FakeClock) error {
	// Contains logic that asynchronously calls clock.Now()
}

func (wm *WorkflowManager) RunWorkflowB(ctx context.Context, clock *FakeClock) error {
	// Contains logic that asynchronously calls clock.Now()
}

// example_test.go
package example_test

import (
	"context"
	"testing"
	"time"

	"example"
)

// Prepare is a test helper function that changes the clock between two steps
// (CreateSomething and CaptureSomething), while background workflows may
// be calling Clock.Now() concurrently.
func Prepare(t *testing.T, createAt, captureAt time.Time) {
	t.Helper()
	ctx := context.Background()

	c := &example.Container{
		Clock:           &example.FakeClock{NowFunc: time.Now},
		WorkflowManager: &example.WorkflowManager{},
	}
	interactor := example.NewInteractor(c)

	// 1) Set clock to createAt
	c.Clock.NowFunc = func() time.Time { return createAt }

	// 2) CreateSomething starts workflow A, which calls c.Clock.Now() asynchronously
	if err := interactor.CreateSomething(ctx); err != nil {
		t.Fatal(err)
	}

	// 3) Switch the clock to captureAt while workflow A is still running in the background
	c.Clock.NowFunc = func() time.Time { return captureAt }

	// 4) CaptureSomething starts workflow B, also calling c.Clock.Now() asynchronously
	if err := interactor.CaptureSomething(ctx); err != nil {
		t.Fatal(err)
	}

	// At this point, if c.Clock.NowFunc was changed while other goroutines
	// were reading it, a data race can happen.
}

It may be difficult to understand the necessity since this is a very simplified example, but when two workflows each execute asynchronous activity functions, the order of mock function calls may not be deterministic.

[breml]

You mention, that workflow A and workflow B are async. So you have no guarantee, that one happens after the other. You also don't have a guarantee, that workflow A already called Now when you change the function in step 3 of prepare. So again, I wonder what exactly your tests can assert for. Maybe it is enough to just return the same time for each call to Now (since you do not check it anyway) or you could just increment the time for each call by some duration or you could use the queue approach I mentioned above where you add createdAt as the first item and captureAt as the second (you will need to extend the queue example I provided with some mutex) and then hope for the best, that the calls happen in the order you expect (first workflow A then workflow B).

For all of this, no change is needed to moq.

[sapuri]

@breml Thank you. This time, it's essentially not a test use case, but a data race is occurring in the Prepare function, making it difficult to convey the necessity.

To summarize this example, the situation is as follows:

• The Create and Capture workflows are executed synchronously, but they include activities that are executed asynchronously.
• createAt and captureAt set in the Prepare function are used to verify the results of Create and Capture, respectively.
• Meanwhile, the asynchronously executed activity also runs Now(), causing a data race here. Since the result of Now() is not used for verifying the result of this activity, the content can be either createAt or captureAt.

Thus, I think that issues like this are quite rare. If there is no demand for the proposed setter, feel free to close this issue.

[breml]

@sapuri If I understand your requirements correctly, you still have the issue of reproducibility, regardless if the data race happens in the Prepare function or during the test it self. If you don't know, in which order the Create and the Capture calls are happening, you will never now, which one will get which date from the mock.

I wonder, if you would be better off, if you inject different mocks for each of the cases.

After all, I am not an official maintainer of moq, so the final call will be with @sudo-suhas or @matryer. But me personally, I am not convinced, that there is actually a demand for the proposed setter.