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

Close the test dispatcher when completing #1117

Closed Groxx closed 3 years ago

Groxx commented 3 years ago

Currently, a test workflow like this will not ever run the code after the goroutine's sleep, nor the code in the defer:

func(ctx workflow.Context) error {
  workflow.Go(func(ctx workflow.Context) {
    defer func() { fmt.Println("in defer") }()
    workflow.Sleep(ctx, time.Hour) // wait longer than the workflow lives
    fmt.Println("after sleep")
  })
  workflow.Sleep(ctx, time.Minute) // just to make sure the goroutine starts
  return nil
}

The workflow will correctly end, but since the dispatcher was never closed, any not-yet-complete goroutines would never exit, and we'd leak goroutines.

Semantically this should be a safe change:

So safe / correct code should be unaffected, leaks should be reduced, and latent mistakes should now cause errors. AFAICT - I'm not sure how complete our tests are here :)

There's some room for in-defer code to be semantically incorrect in tests without this fix, (e.g. testing custom logger/metric impls in defers), though I expect those to be very rare bordering on nonexistent. But for the most part I expect that people will not notice this change, they'll just have fewer goroutine leaks during tests (so e.g. https://github.com/uber-go/goleak users will be happy).


Prior to this fix, the added test fails with:

=== RUN   TestWorkflowUnitTest/Test_StaleGoroutinesAreShutDown
    internal_workflow_test.go:1210: 
            Error Trace:    internal_workflow_test.go:1210
            Error:          deferred func should have been called within 1 second
            Test:           TestWorkflowUnitTest/Test_StaleGoroutinesAreShutDown
    internal_workflow_test.go:1216: code after sleep correctly not executed

Now it passes with this, which also shows it's not slowing tests down in any meaningful way:

=== RUN   TestWorkflowUnitTest/Test_StaleGoroutinesAreShutDown
    internal_workflow_test.go:1210: deferred callback executed after 9.177µs
    internal_workflow_test.go:1217: code after sleep correctly not executed
coveralls commented 3 years ago

Pull Request Test Coverage Report for Build 7f7a0549-3eae-484c-ac63-84b97b1d7cb4


Files with Coverage Reduction New Missed Lines %
internal/internal_task_pollers.go 8 80.52%
<!-- Total: 8 -->
Totals Coverage Status
Change from base Build 6e7ee6fa-6a5e-4c8b-b873-e06b32984d79: -0.05%
Covered Lines: 12067
Relevant Lines: 16978

💛 - Coveralls