Refactor fx Start/Stop usage #4248
Conversation
temporal/fx.go
Outdated
| Logger log.Logger | ||
| ClientFactoryProvider client.FactoryProvider | ||
| DynamicConfigClient dynamicconfig.Client | ||
| DynamicConfigCollection *dynamicconfig.Collection |
There was a problem hiding this comment.
If this is a separate change from Start/Stop, I think it should be its own PR
temporal/fx.go
Outdated
| fx.Populate(&s.blockingStart), | ||
| fx.Populate(&s.logger), |
There was a problem hiding this comment.
The ServerFx object is provided by the NewServerFxImpl provider. We can just inject the blockingStart and logger fields there.
There was a problem hiding this comment.
The ServerFx object is constructed right here. NewServerFxImpl provides a *ServerImpl, not a ServerFx (the names in here are a twisty maze…)
*ServerFx implements Server, but *ServerImpl doesn't. It previously did, but the point was adding contexts, so it can't anymore.
I can rename stuff to make it less confusing but tbh I'm not even sure where to start 😅 Maybe renaming NewServerFxImpl to NewServerImpl? Or ServerImpl to …? And then ServerFx to ServerImpl?
There was a problem hiding this comment.
...let's figure this out later lol
temporal/fx.go
Outdated
| } | ||
|
|
||
| ServerFx struct { | ||
| app *fx.App | ||
| app *fx.App | ||
| blockingStart blockingStartParams |
There was a problem hiding this comment.
nit: I think startupSynchronizationMode with synchronizationModeParams is a bit better
| return ServicesGroupOut{ | ||
| Services: &ServicesMetadata{ | ||
| App: fx.New(fx.NopLogger), | ||
| ServiceName: serviceName, | ||
| ServiceStopFn: func() {}, | ||
| }, | ||
| }, nil |
There was a problem hiding this comment.
This is a great change, but can we move it to a separate PR?
There was a problem hiding this comment.
I split out the Collection one (already in a separate commit) but I'd rather not spend time on splitting the rest, they're fairly intertwined
This reverts commit 661d6d4.
What changed?
This leaves the shutdown timeout in the inner Apps: if we set a single timeout on the outer one, fx itself might abort shutdown and not give the inner ones time to finish, so I think it's more robust this way.
Why?
Simplify and correct usage of fx
How did you test it?
Local testing, integration tests
Potential risks
Is hotfix candidate?