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.

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

fatal error: concurrent map writes when u #1703

Open
delanne opened this issue Nov 5, 2024 · 2 comments · May be fixed by #1704
Open

fatal error: concurrent map writes when u #1703

delanne opened this issue Nov 5, 2024 · 2 comments · May be fixed by #1704

Comments

@delanne
Copy link

delanne commented Nov 5, 2024

Expected Behavior

  • no crash

Actual Behavior

when testing several Activities with t.Parallel, the test framework crash with "fatal error: concurrent map writes"

fatal error: concurrent map writes
...
goroutine 129 [running]:
go.temporal.io/sdk/internal.(*testWorkflowEnvironmentImpl).setActivityHandle(...)
	/Users/xavier.delannoy/go/pkg/mod/go.temporal.io/[email protected]/internal/internal_workflow_testsuite.go:1415
go.temporal.io/sdk/internal.(*testWorkflowEnvironmentImpl).executeActivity(0x140000e4488, {0x10304e900?, 0x1031d92a8?}, {0x14000513020, 0x1, 0x1})
	/Users/xavier.delannoy/go/pkg/mod/go.temporal.io/[email protected]/internal/internal_workflow_testsuite.go:703 +0x524
go.temporal.io/sdk/internal.(*TestActivityEnvironment).ExecuteActivity(0x1030c99a0?, {0x10304e900?, 0x1031d92a8?}, {0x14000513020?, 0x140004d1f38?, 0x1028bb6a0?})
	/Users/xavier.delannoy/go/pkg/mod/go.temporal.io/[email protected]/internal/workflow_testsuite.go:195 +0x2c
github.com/strangebee/godsl/flow/activities/strings.Test_Activity_TrimRight.func1(0x140004c36c0)
	/Users/xavier.delannoy/go/src/github.com/strangebee/Delannoy/godsl/flow/activities/strings/trimright_test.go:150 +0xac
testing.tRunner(0x140004c36c0, 0x14000494390)
	/usr/local/go/src/testing/testing.go:1690 +0xe4
created by testing.(*T).Run in goroutine 24
	/usr/local/go/src/testing/testing.go:1743 +0x314

Steps to Reproduce the Problem

  1. write some activities
  2. write tests for these activities with t.Parallel
  3. run tests in a loop in order to reproduce the race conditions

Specifications

  • Version: SDK 1.30
  • Platform: Darwin

Investigation

code:

func (env *testWorkflowEnvironmentImpl) setActivityHandle(activityID, runID string, handle *testActivityHandle) {
	env.activities[env.makeUniqueActivityID(activityID, runID)] = handle
}
		activities             map[string]*testActivityHandle

as several go routines can write in the map 'activities', the type of activities should be a sync.Map or use a mutex to protect the write

Potential fix

internal/internal_workflow_testsuite.go:1415

func (env *testWorkflowEnvironmentImpl) setActivityHandle(activityID, runID string, handle *testActivityHandle) {
	env.locker.Lock()
	defer env.locker.Unlock()
	env.activities[env.makeUniqueActivityID(activityID, runID)] = handle
}
delanne added a commit to delanne/sdk-go that referenced this issue Nov 5, 2024
protect the write to the map env.activities with env.locker
@delanne delanne linked a pull request Nov 5, 2024 that will close this issue
@Quinn-With-Two-Ns
Copy link
Contributor

To clarify, are you seeing this data race when the same test environment being shared by multiple tests?

@delanne
Copy link
Author

delanne commented Nov 7, 2024

To clarify, are you seeing this data race when the same test environment being shared by multiple tests?

all tests are run in parallel. Here is an example of how I write a test

func Test_Activity_FooBar(t *testing.T) {
	testSuite := &testsuite.WorkflowTestSuite{}
	env := testSuite.NewTestActivityEnvironment()

	env.RegisterActivity(FooBar)

	t.Parallel()
	tests := []struct {
		name     string
		p        *FooBarParams
		expected internal.Return
	}{
		{
			name: "foobar test1",
			p: &FooBarParams{
				String1: "foobar",
				String2: "foobar",
			},
			expected: internal.Return{Value: float64(0)},
		},
     .....
	}

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			t.Parallel()
			val, err := env.ExecuteActivity(FooBar,
				tt.p,
			)
			require.NoError(t, err)
			res := &internal.Return{}
			err = val.Get(res)
			require.NoError(t, err)
			require.Equal(t, &tt.expected, res)
		})
	}
}

Do you mean that the line env := testSuite.NewTestActivityEnvironment() should be after the line t.Parallel() ? (make sense to me)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants