uber-go / fx

A dependency injection based application framework for Go.
https://uber-go.github.io/fx/
MIT License
5.48k stars 283 forks source link

fxtest.Lifecycle: Enforce timeout #1180

Closed abhinav closed 1 month ago

abhinav commented 3 months ago

Is your feature request related to a problem? Please describe.

fx.App.Start and fx.App.Stop enforce the context timeout on start/stop hooks by spawning a goroutine: https://github.com/uber-go/fx/blob/v1.21.0/app.go#L726-L731

fxtest.LIfecycle is supposed to be a lifecycle implementation meant for use in tests. However, if one runs lifecycle.Start or lifecycle.Stop on it with an indefinitely blocking operation, the lifecycle will never resolve.

lc := fxtest.NewLifecycle()
lc.Append(fx.StopHook(func() {
  select {}
}))

ctx, cancel := context.WithTimeout(context.Background(), time.Second)
err := lc.Stop(ctx) // blocks forever

This does not match the behavior of fx.App, where the lifecycle will abort early and return an error.

Describe the solution you'd like

fxtest.Lifecycle should similarly enforce the context timeout on the lifecycle operations.

Describe alternatives you've considered

Spawn a goroutine from Start/Stop operation to manually enforce the timeout. This feels unnecessary/incorrect because you don't need to do it for hooks when they're called from App.Start/App.Stop, but do need to do it for Lifecycle.Start/Stop.

Is this a breaking change? This is not a breaking change to the API. However, there's a possibility that someone was relying on the lifecycle to block previously, and their tests could begin failing.

One argument is that if their tests were violating the timeout anyway, that's likely a bug. However, if we don't feel comfortable making this change in that way, we can instead use an option or a different constructor to turn this behavior on/off.