Commit b2db20e
committed
Close the test dispatcher when completing (#1117)
Currently, a test workflow like this will not ever run the code after the goroutine's sleep (correct),
nor the code in the defer (incorrect, production would run it):
```
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:
- Any post-complete decisions would not be executed or recorded, and this retains that.
- When panicking, anything that would be *recorded* in a defer will not be recorded, so no
replay-state-aware user code should be affected. And any code that ignores replay state will
now execute like it should, where before it would not.
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
```1 parent 2017e60 commit b2db20e
File tree
2 files changed
+38
-0
lines changed- internal
2 files changed
+38
-0
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1181 | 1181 | | |
1182 | 1182 | | |
1183 | 1183 | | |
| 1184 | + | |
| 1185 | + | |
| 1186 | + | |
| 1187 | + | |
| 1188 | + | |
| 1189 | + | |
| 1190 | + | |
| 1191 | + | |
| 1192 | + | |
| 1193 | + | |
| 1194 | + | |
| 1195 | + | |
| 1196 | + | |
| 1197 | + | |
| 1198 | + | |
| 1199 | + | |
| 1200 | + | |
| 1201 | + | |
| 1202 | + | |
| 1203 | + | |
| 1204 | + | |
| 1205 | + | |
| 1206 | + | |
| 1207 | + | |
| 1208 | + | |
| 1209 | + | |
| 1210 | + | |
| 1211 | + | |
| 1212 | + | |
| 1213 | + | |
| 1214 | + | |
| 1215 | + | |
| 1216 | + | |
| 1217 | + | |
| 1218 | + | |
| 1219 | + | |
| 1220 | + | |
1184 | 1221 | | |
1185 | 1222 | | |
1186 | 1223 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
818 | 818 | | |
819 | 819 | | |
820 | 820 | | |
| 821 | + | |
821 | 822 | | |
822 | 823 | | |
823 | 824 | | |
| |||
0 commit comments