From 95cbe8346583b3824d4cb46de94dc60b7799ad09 Mon Sep 17 00:00:00 2001 From: Jacob Oaks Date: Fri, 19 Apr 2024 12:37:39 -0400 Subject: [PATCH] Provide Fx types before user types (#1191) Consider the following example: ```go func opts() fx.Option { return fx.Options( fx.WithLogger(func(fx.Lifecycle) fxevent.Logger { return &fxevent.ConsoleLogger{ W: os.Stdout } }), fx.Provide(func() string { return "" }), fx.Provide(func() string { return "" }), ) } func main() { fx.New(opts()).Run() } ``` The relevant issue to surface to the user is that they are double providing the same type. However, the actual error message is: ``` [Fx] ERROR Failed to start: the following errors occurred: - fx.Provide(main.opts.func3()) from: main.opts /home/user/go/src/scratch/fx_provide_order/main.go:17 main.main /home/user/go/src/scratch/fx_provide_order/main.go:22 runtime.main /opt/go/root/src/runtime/proc.go:271 Failed: cannot provide function "main".opts.func3 (/home/user/go/src/scratch/fx_provide_order/main.go:17): cannot provide string from [0]: already provided by "main".opts.func2 (/home/user/go/src/scratch/fx_provide_order/main.go:16) - could not build arguments for function "go.uber.org/fx".(*module).constructCustomLogger.func2 /home/user/go-repos/pkg/mod/go.uber.org/fx@v1.21.0/module.go:292: failed to build fxevent.Logger: missing dependencies for function "main".opts.func1 /home/user/go/src/scratch/fx_provide_order/main.go:11: missing type: - fx.Lifecycle (did you mean to Provide it?) ``` Which contains an additional error related to how the custom logger could not be built. This is because Fx will try to continue to build the custom logger in the face of DI failure, theoretically to report issues through the right channels. But after an error occurs when providing anything, [Fx refuses to provide any more types](https://github.com/uber-go/fx/blob/master/module.go#L184) - leading to a subsequent error when trying to build this custom logger that depends on the `fx.Lifecycle` type. This is a common issue that can be misleading for new engineers debugging their fx apps. I couldn't find any particular reason why user-provided provides are registered before these Fx types, so this PR switches this ordering so that custom loggers can still be built if they rely on the Fx types, which cleans up the error message: ``` [Fx] ERROR Failed to start: fx.Provide(main.opts.func3()) from: main.opts /home/user/go/src/scratch/fx_provide_order/main.go:17 main.main /home/user/go/src/scratch/fx_provide_order/main.go:22 runtime.main /opt/go/root/src/runtime/proc.go:271 Failed: cannot provide function "main".opts.func3 (/home/user/go/src/scratch/fx_provide_order/main.go:17): cannot provide string from [0]: already provided by "main".opts.func2 (/home/user/go/src/scratch/fx_provide_order/main.go:16) ``` --- app.go | 11 +++++++---- app_test.go | 17 ++++++++++++----- module_test.go | 10 +++++----- 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/app.go b/app.go index 8ede19eb5..8189f9d71 100644 --- a/app.go +++ b/app.go @@ -480,10 +480,9 @@ func New(opts ...Option) *App { m.build(app, app.container) } - for _, m := range app.modules { - m.provideAll() - } - + // Provide Fx types first to increase the chance a custom logger + // can be successfully built in the face of unrelated DI failure. + // E.g., for a custom logger that relies on the Lifecycle type. frames := fxreflect.CallerStack(0, 0) // include New in the stack for default Provides app.root.provide(provide{ Target: func() Lifecycle { return app.lifecycle }, @@ -492,6 +491,10 @@ func New(opts ...Option) *App { app.root.provide(provide{Target: app.shutdowner, Stack: frames}) app.root.provide(provide{Target: app.dotGraph, Stack: frames}) + for _, m := range app.modules { + m.provideAll() + } + // Run decorators before executing any Invokes -- including the one // inside constructCustomLogger. app.err = multierr.Append(app.err, app.root.decorateAll()) diff --git a/app_test.go b/app_test.go index 2ac6ef2bf..cc17a4f19 100644 --- a/app_test.go +++ b/app_test.go @@ -113,7 +113,13 @@ func TestNewApp(t *testing.T) { []string{"Provided", "Provided", "Provided", "Provided", "LoggerInitialized", "Started"}, spy.EventTypes()) - assert.Contains(t, spy.Events()[0].(*fxevent.Provided).OutputTypeNames, "struct {}") + // Fx types get provided first to increase chance of + // successful custom logger build. + assert.Contains(t, spy.Events()[0].(*fxevent.Provided).OutputTypeNames, "fx.Lifecycle") + assert.Contains(t, spy.Events()[1].(*fxevent.Provided).OutputTypeNames, "fx.Shutdowner") + assert.Contains(t, spy.Events()[2].(*fxevent.Provided).OutputTypeNames, "fx.DotGraph") + // Our type should be index 3. + assert.Contains(t, spy.Events()[3].(*fxevent.Provided).OutputTypeNames, "struct {}") }) t.Run("CircularGraphReturnsError", func(t *testing.T) { @@ -575,7 +581,7 @@ func TestWithLogger(t *testing.T) { ) assert.Equal(t, []string{ - "Supplied", "Provided", "Provided", "Provided", "Run", "LoggerInitialized", + "Provided", "Provided", "Provided", "Supplied", "Run", "LoggerInitialized", }, spy.EventTypes()) spy.Reset() @@ -605,7 +611,7 @@ func TestWithLogger(t *testing.T) { "must provide constructor function, got (type *bytes.Buffer)", ) - assert.Equal(t, []string{"Supplied", "Provided", "Run", "LoggerInitialized"}, spy.EventTypes()) + assert.Equal(t, []string{"Provided", "Provided", "Provided", "Supplied", "Provided", "Run", "LoggerInitialized"}, spy.EventTypes()) }) t.Run("logger failed to build", func(t *testing.T) { @@ -1166,8 +1172,9 @@ func TestOptions(t *testing.T) { Provide(&bytes.Buffer{}), // error, not a constructor WithLogger(func() fxevent.Logger { return spy }), ) - require.Equal(t, []string{"Provided", "LoggerInitialized"}, spy.EventTypes()) - assert.Contains(t, spy.Events()[0].(*fxevent.Provided).Err.Error(), "must provide constructor function") + require.Equal(t, []string{"Provided", "Provided", "Provided", "Provided", "LoggerInitialized"}, spy.EventTypes()) + // First 3 provides are Fx types (Lifecycle, Shutdowner, DotGraph). + assert.Contains(t, spy.Events()[3].(*fxevent.Provided).Err.Error(), "must provide constructor function") }) } diff --git a/module_test.go b/module_test.go index 1a6fd9b71..3ac20527b 100644 --- a/module_test.go +++ b/module_test.go @@ -261,7 +261,7 @@ func TestModuleSuccess(t *testing.T) { desc: "custom logger for module", giveWithLogger: fx.NopLogger, wantEvents: []string{ - "Supplied", "Provided", "Provided", "Provided", + "Provided", "Provided", "Provided", "Supplied", "Run", "LoggerInitialized", "Invoking", "Invoked", }, }, @@ -269,7 +269,7 @@ func TestModuleSuccess(t *testing.T) { desc: "Not using a custom logger for module defaults to app logger", giveWithLogger: fx.Options(), wantEvents: []string{ - "Supplied", "Provided", "Provided", "Provided", "Provided", "Run", + "Provided", "Provided", "Provided", "Supplied", "Provided", "Run", "LoggerInitialized", "Invoking", "Run", "Invoked", "Invoking", "Invoked", }, }, @@ -660,7 +660,7 @@ func TestModuleFailures(t *testing.T) { giveAppOpts: spyAsLogger, wantErrContains: []string{"error building logger"}, wantEvents: []string{ - "Supplied", "Provided", "Provided", "Provided", "Run", + "Provided", "Provided", "Provided", "Supplied", "Run", "LoggerInitialized", "Provided", "LoggerInitialized", }, }, @@ -678,7 +678,7 @@ func TestModuleFailures(t *testing.T) { giveAppOpts: spyAsLogger, wantErrContains: []string{"error building logger dependency"}, wantEvents: []string{ - "Supplied", "Provided", "Provided", "Provided", "Run", + "Provided", "Provided", "Provided", "Supplied", "Run", "LoggerInitialized", "Provided", "Provided", "Run", "LoggerInitialized", }, }, @@ -690,7 +690,7 @@ func TestModuleFailures(t *testing.T) { "fx.WithLogger", "from:", "Failed", }, wantEvents: []string{ - "Supplied", "Provided", "Provided", "Provided", "Run", + "Provided", "Provided", "Provided", "Supplied", "Run", "LoggerInitialized", "Provided", "LoggerInitialized", }, },