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.

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

fxtest.Lifecycle: Enforce timeout #1180

Open
abhinav opened this issue Mar 20, 2024 · 0 comments
Open

fxtest.Lifecycle: Enforce timeout #1180

abhinav opened this issue Mar 20, 2024 · 0 comments

Comments

@abhinav
Copy link
Collaborator

abhinav commented Mar 20, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants