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.

JacobOaks added a commit to JacobOaks/fx that referenced this issue May 21, 2024
In Fx, if starting/stopping the application takes longer than the context's timeout,
the fx.App will bail early, regardless of the status of the currently running hooks.

This prevents stalling an application when hooks (accidentally) block forever.

In order to test hook behavior, Fx provides fxtest.Lifecycle to interact with.
This Lifecycle is a simple wrapper around the actual fx Lifecycle type,
meaning it does not check for timeout and bail early like fx.App does.
This is an issue because:
* It allows for long-running hooks (which should be considered bugs) to pass tests.
* It allows for tests to completely stall for hooks that block forever.

See uber-go#1180 for more details.

This PR adds an option that can be passed to `fxtest.NewLifecycle`
to cause it to immediately fail when context expires, similar to fx.App.

```
lc := fxtest.NewLifecycle(fxtest.EnforceTimeout(true))
lc.Append(fx.StartHook(func() { for {} }))
ctx, _ := context.WithTimeout(context.Background(), time.Second)
err := lc.Start(ctx) // Will return deadline exceeded after a second
```

This PR doesn't provide a way to test timeouts using `RequireStart` and
`RequireStop`. However, since those don't take contexts anyways,
my assumption is that usage of those APIs represents an intentional decision
to not care about timeouts by the test writer.

However, if people feel differently, we can instead do something like expose two new APIs
`RequireStartWithContext(ctx)` and `RequireStopWithContext(ctx)` or something
(names obviously up for discussion).
JacobOaks added a commit to JacobOaks/fx that referenced this issue May 21, 2024
In Fx, if starting/stopping the application takes longer than the context's timeout,
the fx.App will bail early, regardless of the status of the currently running hooks.

This prevents stalling an application when hooks (accidentally) block forever.

In order to test hook behavior, Fx provides fxtest.Lifecycle to interact with.
This Lifecycle is a simple wrapper around the actual fx Lifecycle type,
meaning it does not check for timeout and bail early like fx.App does.
This is an issue because:
* It allows for long-running hooks (which should be considered bugs) to pass tests.
* It allows for tests to completely stall for hooks that block forever.

See uber-go#1180 for more details.

This PR adds an option that can be passed to `fxtest.NewLifecycle`
to cause it to immediately fail when context expires, similar to fx.App.

```
lc := fxtest.NewLifecycle(fxtest.EnforceTimeout(true))
lc.Append(fx.StartHook(func() { for {} }))
ctx, _ := context.WithTimeout(context.Background(), time.Second)
err := lc.Start(ctx) // Will return deadline exceeded after a second
```

This PR doesn't provide a way to test timeouts using `RequireStart` and
`RequireStop`. However, since those don't take contexts anyways,
my assumption is that usage of those APIs represents an intentional decision
to not care about timeouts by the test writer.

However, if people feel differently, we can instead do something like expose two new APIs
`RequireStartWithContext(ctx)` and `RequireStopWithContext(ctx)` or something
(names obviously up for discussion).
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