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.

Sign up for GitHub

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

Jump to bottom

fatal error: concurrent map writes when u #1703

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

fatal error: concurrent map writes when u #1703

delanne opened this issue Nov 5, 2024 · 3 comments · May be fixed by #1704
Labels
potential-bug

Comments

@delanne
Copy link

delanne commented Nov 5, 2024 •
edited
Loading

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
@delanne
Fix issue temporalio#1703
69b2c64 
protect the write to the map env.activities with env.locker
@delanne delanne mentioned this issue Nov 5, 2024
Open
@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)

@Quinn-With-Two-Ns
Copy link
Contributor

Sorry for the late reply this slipped through the cracks ,

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

Yes, The test environment should not be shared for multiple tests. Each test should have its own environment

@Quinn-With-Two-Ns Quinn-With-Two-Ns closed this as not planned Won't fix, can't repro, duplicate, stale Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
potential-bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix issue #1703 delanne/sdk-go
2 participants
@delanne @Quinn-With-Two-Ns