uber-go / cadence-client

Framework for authoring workflows and activities running on top of the Cadence orchestration engine.
https://cadenceworkflow.io
MIT License
345 stars 131 forks source link

Fix client side race condition #1124

Closed demirkayaender closed 3 years ago

demirkayaender commented 3 years ago

What changed? The problem is described here: https://github.com/uber-go/cadence-client/pull/1117

That PR fixes leaking go routines but also introduces a race condition, this PR fixes the race.

Why?

How did you test it?

go test -race -test.count 1 -test.run TestContext_RaceRegression ./internal
go test -timeout 30s -run ^TestWorkflowUnitTest$ -testify.m ^(Test_StaleGoroutinesAreShutDown)$ go.uber.org/cadence/internal

Output of the first test without the fix:

==================
WARNING: DATA RACE
Read at 0x00c0003feaa8 by goroutine 13:
  go.uber.org/cadence/internal.(*cancelCtx).cancel()
      /Users/enderd/cadence-client/internal/context.go:312 +0x71
  go.uber.org/cadence/internal.WithCancel.func1()
      /Users/enderd/cadence-client/internal/context.go:194 +0x78
  runtime.call16()
      /usr/local/Cellar/go/1.16.6/libexec/src/runtime/asm_amd64.s:550 +0x3d
  go.uber.org/cadence/internal.(*coroutineState).initialYield()
      /Users/enderd/cadence-client/internal/internal_workflow.go:796 +0xb9
  go.uber.org/cadence/internal.(*coroutineState).yield()
      /Users/enderd/cadence-client/internal/internal_workflow.go:805 +0x45c
  go.uber.org/cadence/internal.(*channelImpl).Receive()
      /Users/enderd/cadence-client/internal/internal_workflow.go:620 +0x35a
  go.uber.org/cadence/internal.(*futureImpl).Get()
      /Users/enderd/cadence-client/internal/internal_workflow.go:294 +0x92
  go.uber.org/cadence/internal.(*workflowEnvironmentInterceptor).Sleep()
      /Users/enderd/cadence-client/internal/workflow.go:906 +0x81
  go.uber.org/cadence/internal.Sleep()
      /Users/enderd/cadence-client/internal/workflow.go:901 +0xc1
  go.uber.org/cadence/internal.TestContext_RaceRegression.func1.1()
      /Users/enderd/cadence-client/internal/context_test.go:49 +0x99
  go.uber.org/cadence/internal.(*dispatcherImpl).newNamedCoroutine.func1()
      /Users/enderd/cadence-client/internal/internal_workflow.go:883 +0x11c

Previous write at 0x00c0003feaa8 by goroutine 16:
  go.uber.org/cadence/internal.(*cancelCtx).cancel()
      /Users/enderd/cadence-client/internal/context.go:315 +0x93
  go.uber.org/cadence/internal.WithCancel.func1()
      /Users/enderd/cadence-client/internal/context.go:194 +0x78
  runtime.call16()
      /usr/local/Cellar/go/1.16.6/libexec/src/runtime/asm_amd64.s:550 +0x3d
  go.uber.org/cadence/internal.(*coroutineState).initialYield()
      /Users/enderd/cadence-client/internal/internal_workflow.go:796 +0xb9
  go.uber.org/cadence/internal.(*coroutineState).yield()
      /Users/enderd/cadence-client/internal/internal_workflow.go:805 +0x45c
  go.uber.org/cadence/internal.(*channelImpl).Receive()
      /Users/enderd/cadence-client/internal/internal_workflow.go:620 +0x35a
  go.uber.org/cadence/internal.(*futureImpl).Get()
      /Users/enderd/cadence-client/internal/internal_workflow.go:294 +0x92
  go.uber.org/cadence/internal.(*workflowEnvironmentInterceptor).Sleep()
      /Users/enderd/cadence-client/internal/workflow.go:906 +0x81
  go.uber.org/cadence/internal.Sleep()
      /Users/enderd/cadence-client/internal/workflow.go:901 +0xc1
  go.uber.org/cadence/internal.TestContext_RaceRegression.func1.1()
      /Users/enderd/cadence-client/internal/context_test.go:49 +0x99
  go.uber.org/cadence/internal.(*dispatcherImpl).newNamedCoroutine.func1()
      /Users/enderd/cadence-client/internal/internal_workflow.go:883 +0x11c

Goroutine 13 (running) created at:
  go.uber.org/cadence/internal.(*dispatcherImpl).newNamedCoroutine()
      /Users/enderd/cadence-client/internal/internal_workflow.go:874 +0x3d1
  go.uber.org/cadence/internal.(*dispatcherImpl).newCoroutine()
      /Users/enderd/cadence-client/internal/internal_workflow.go:868 +0xeb
  go.uber.org/cadence/internal.Go()
      /Users/enderd/cadence-client/internal/workflow.go:394 +0x94
  go.uber.org/cadence/internal.TestContext_RaceRegression.func1()
      /Users/enderd/cadence-client/internal/context_test.go:53 +0xe9
  runtime.call32()
      /usr/local/Cellar/go/1.16.6/libexec/src/runtime/asm_amd64.s:551 +0x3d
  reflect.Value.Call()
      /usr/local/Cellar/go/1.16.6/libexec/src/reflect/value.go:337 +0xd8
  go.uber.org/cadence/internal.(*workflowEnvironmentInterceptor).ExecuteWorkflow()
      /Users/enderd/cadence-client/internal/workflow.go:423 +0x2e4
  go.uber.org/cadence/internal.(*workflowExecutor).Execute()
      /Users/enderd/cadence-client/internal/internal_worker.go:700 +0x41a
  go.uber.org/cadence/internal.(*workflowExecutorWrapper).Execute()
      /Users/enderd/cadence-client/internal/internal_workflow_testsuite.go:1459 +0x8f9
  go.uber.org/cadence/internal.(*syncWorkflowDefinition).Execute.func1()
      /Users/enderd/cadence-client/internal/internal_workflow.go:466 +0x243
  go.uber.org/cadence/internal.(*dispatcherImpl).newNamedCoroutine.func1()
      /Users/enderd/cadence-client/internal/internal_workflow.go:883 +0x11c

Goroutine 16 (running) created at:
  go.uber.org/cadence/internal.(*dispatcherImpl).newNamedCoroutine()
      /Users/enderd/cadence-client/internal/internal_workflow.go:874 +0x3d1
  go.uber.org/cadence/internal.(*dispatcherImpl).newCoroutine()
      /Users/enderd/cadence-client/internal/internal_workflow.go:868 +0xeb
  go.uber.org/cadence/internal.Go()
      /Users/enderd/cadence-client/internal/workflow.go:394 +0x94
  go.uber.org/cadence/internal.TestContext_RaceRegression.func1()
      /Users/enderd/cadence-client/internal/context_test.go:53 +0xe9
  runtime.call32()
      /usr/local/Cellar/go/1.16.6/libexec/src/runtime/asm_amd64.s:551 +0x3d
  reflect.Value.Call()
      /usr/local/Cellar/go/1.16.6/libexec/src/reflect/value.go:337 +0xd8
  go.uber.org/cadence/internal.(*workflowEnvironmentInterceptor).ExecuteWorkflow()
      /Users/enderd/cadence-client/internal/workflow.go:423 +0x2e4
  go.uber.org/cadence/internal.(*workflowExecutor).Execute()
      /Users/enderd/cadence-client/internal/internal_worker.go:700 +0x41a
  go.uber.org/cadence/internal.(*workflowExecutorWrapper).Execute()
      /Users/enderd/cadence-client/internal/internal_workflow_testsuite.go:1459 +0x8f9
  go.uber.org/cadence/internal.(*syncWorkflowDefinition).Execute.func1()
      /Users/enderd/cadence-client/internal/internal_workflow.go:466 +0x243
  go.uber.org/cadence/internal.(*dispatcherImpl).newNamedCoroutine.func1()
      /Users/enderd/cadence-client/internal/internal_workflow.go:883 +0x11c
==================
FAIL
FAIL    go.uber.org/cadence/internal    0.646s
FAIL

Potential risks

coveralls commented 3 years ago

Pull Request Test Coverage Report for Build a606b89c-de6f-41f7-a1d2-e8199e40e732


Files with Coverage Reduction New Missed Lines %
internal/context.go 2 78.29%
<!-- Total: 2 -->
Totals Coverage Status
Change from base Build 00905a6a-bf4b-4d84-b4f3-f5ea4534cfdc: 0.02%
Covered Lines: 12158
Relevant Lines: 16994

💛 - Coveralls