Skip to content

Commit

Permalink
Provide internal types first
Browse files Browse the repository at this point in the history
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 - 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, se 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)
```
  • Loading branch information
JacobOaks committed Apr 18, 2024
1 parent 9814dd3 commit a7bed35
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 14 deletions.
8 changes: 4 additions & 4 deletions app.go
Original file line number Diff line number Diff line change
Expand Up @@ -480,10 +480,6 @@ func New(opts ...Option) *App {
m.build(app, app.container)
}

for _, m := range app.modules {
m.provideAll()
}

frames := fxreflect.CallerStack(0, 0) // include New in the stack for default Provides
app.root.provide(provide{
Target: func() Lifecycle { return app.lifecycle },
Expand All @@ -492,6 +488,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())
Expand Down
34 changes: 29 additions & 5 deletions app_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,30 @@ func TestNewApp(t *testing.T) {
[]string{"Provided", "Provided", "Provided", "Provided", "LoggerInitialized", "Started"},
spy.EventTypes())

assert.Contains(t, spy.Events()[0].(*fxevent.Provided).OutputTypeNames, "struct {}")
// Lifecycle, Shutdowner, and DotGraph are indices 0, 1, and 2.
// Our type should be index 3.
assert.Contains(t, spy.Events()[3].(*fxevent.Provided).OutputTypeNames, "struct {}")
})

t.Run("InternalTypesProvidedFirst", func(t *testing.T) {
t.Parallel()

// Test that internal types get provided before
// user-specified types. This increases the chance
// that custom loggers relying on internal types
// can be built successfully.

spy := new(fxlog.Spy)
app := fxtest.New(t, Provide(func() struct{} { return struct{}{} }),
WithLogger(func() fxevent.Logger { return spy }))
defer app.RequireStart().RequireStop()
require.Equal(t,
[]string{"Provided", "Provided", "Provided", "Provided", "LoggerInitialized", "Started"},
spy.EventTypes())

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")
})

t.Run("CircularGraphReturnsError", func(t *testing.T) {
Expand Down Expand Up @@ -575,7 +598,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()
Expand Down Expand Up @@ -605,7 +628,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) {
Expand Down Expand Up @@ -1166,8 +1189,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")
})
}

Expand Down
10 changes: 5 additions & 5 deletions module_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -261,15 +261,15 @@ 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",
},
},
{
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",
},
},
Expand Down Expand Up @@ -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",
},
},
Expand All @@ -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",
},
},
Expand All @@ -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",
},
},
Expand Down

0 comments on commit a7bed35

Please sign in to comment.