fx.OnStart dependency missing, sometimes
advdv opened this issue · 3 comments
Describe the bug
fx.OnStart is not able to resolve my *net.TCPListener dependency in the expected way. It does work in an unexpected way, see below for explanation.
To Reproduce
Let's consider the example below for a webserver module.
// Package webserver implements the http serving
package webserver
import (
"context"
"fmt"
"net"
"net/http"
"net/netip"
"go.uber.org/fx"
)
// NewListener provides a tcp connection listener for the webserver.
func NewListener() (*net.TCPListener, error) {
ap, err := netip.ParseAddrPort("[::1]:8383")
if err != nil {
return nil, fmt.Errorf("failed to parse addr/port: %w", err)
}
ln, err := net.ListenTCP("tcp", net.TCPAddrFromAddrPort(ap))
if err != nil {
return nil, fmt.Errorf("failed to listen: %w", err)
}
return ln, nil
}
// New inits the http server.
func New(h http.Handler) *http.Server {
return &http.Server{
Handler: h,
}
}
// moduleName standardizes the module name.
const moduleName = "webhandler"
// Prod dependencies.
func Prod() fx.Option {
return fx.Module(moduleName,
fx.Provide(fx.Annotate(NewListener)),
fx.Provide(fx.Annotate(New,
fx.OnStart(func(ctx context.Context, ln *net.TCPListener, s *http.Server) error {
go s.Serve(ln) //nolint:errcheck
return nil
}),
fx.OnStop(func(ctx context.Context, s *http.Server) error {
if err := s.Shutdown(ctx); err != nil {
return fmt.Errorf("failed to shut down: %w", err)
}
return nil
}))),
)
}Given a test file like this:
package webserver
import (
"context"
"net/http"
"testing"
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
"go.uber.org/fx"
)
func TestWebserver(t *testing.T) {
t.Parallel()
RegisterFailHandler(Fail)
RunSpecs(t, "web/webserver")
}
var _ = Describe("failed to handle webserver", func() {
BeforeEach(func(ctx context.Context) {
app := fx.New(Prod(),
fx.Invoke(func(s *http.Server) {}),
fx.Supply(fx.Annotate(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}),
fx.As(new(http.Handler)))))
Expect(app.Start(ctx)).To(Succeed())
DeferCleanup(app.Done)
})
It("should server http", func() {
// ....
})
})Running go test will result in an error like this:
error invoking hook installer: missing dependencies for function \"reflect\".makeFuncStub (/usr/local/go/src/reflect/asm_arm64.s:29): missing type: *net.TCPListener",
But, if I change the signature of the New function, like this:
// Package webserver implements the http serving
package webserver
import (
"context"
"fmt"
"net"
"net/http"
"net/netip"
"go.uber.org/fx"
)
// NewListener provides a tcp connection listener for the webserver.
func NewListener() (*net.TCPListener, error) {
ap, err := netip.ParseAddrPort("[::1]:8383")
if err != nil {
return nil, fmt.Errorf("failed to parse addr/port: %w", err)
}
ln, err := net.ListenTCP("tcp", net.TCPAddrFromAddrPort(ap))
if err != nil {
return nil, fmt.Errorf("failed to listen: %w", err)
}
return ln, nil
}
// New inits the http server. NOTE: I've added the *net.TCPListener as a dependency even though
// we don't need it to make the *http.Server
func New(h http.Handler, _ *net.TCPListener) *http.Server {
return &http.Server{
Handler: h,
}
}
// moduleName standardizes the module name.
const moduleName = "webhandler"
// Prod dependencies.
func Prod() fx.Option {
return fx.Module(moduleName,
fx.Provide(fx.Annotate(NewListener)),
fx.Provide(fx.Annotate(New,
fx.OnStart(func(ctx context.Context, ln *net.TCPListener, s *http.Server) error {
go s.Serve(ln) //nolint:errcheck
return nil
}),
fx.OnStop(func(ctx context.Context, s *http.Server) error {
if err := s.Shutdown(ctx); err != nil {
return fmt.Errorf("failed to shut down: %w", err)
}
return nil
}))),
)
}It will work.
Expected behavior
I would expect the first setup above to work without missing dependency error.
Additional context
I'm pretty sure this used to work in the past since I THINK I've written this code before.
Hey @advanderveer,
Hooks passed as OnStart hooks are not allowed to take dependencies outside of the provider's direct dependencies or result types.
See: https://pkg.go.dev/go.uber.org/fx#OnStart:
"The hook function passed into OnStart cannot take any arguments outside of the annotated constructor's existing dependencies or results, except a context.Context."
I'm pretty sure this used to work in the past since I THINK I've written this code before.
That was actually an unintentional effect of a bug that was fixed in #983.
That explains it then, thank you for the clear response. I guess a more clear error would be better (it could be detected that a requested dependency is invalid) but since it is already documented I guess this can also be closed.