uber-go / fx

A dependency injection based application framework for Go.
https://uber-go.github.io/fx/
MIT License
5.65k stars 287 forks source link

Using context when providing a new dependency #879

Open screwyprof opened 2 years ago

screwyprof commented 2 years ago

I've got a couple of use cases when I need to pass context.Context to the a contractor function.

For example, in one of the project I'm currently working on we have a function to establish a connection with a database, which uses a backoff retry strategy under the hood. The retries are controlled via context. If the context is cancelled, the function which is being retried will exit.

Similarly, we've got an authenticator component, which has to load a jwk set via http. It has a similar retry function to handle possible http failures.

Is there any way to pass/or use application start context in the constructor?

Below is some simplified function to demonstrate the issue:

func  NewAuthenicator(ctx context.Context, jwksURL string, log logr.Logger) (*Authenticator, error) {
    const maxRetries = 10

    var jwkSet  jwk.Set

    backoff := retry.WithMaxRetries(maxRetries, retry.NewExponential(time.Second))
    retryFn := func(ctx context.Context) error {
                var err error

        log.Info("fetching jwk set", "url", jwksURL)

        jwkSet, err = jwk.Fetch(ctx, jwksURL)
        if err != nil {
            return retry.RetryableError(err) 
        }

        return nil
    }

    if err := retry.Do(ctx, backoff, retryFn); err != nil {
        return nil, err
    }

    return Authenticator{jwkSet: jwkSet, log: log}, nil
}
Southclaws commented 2 years ago

For this, I believe the recommended way is to use OnStart and OnStop hooks with your own context which is cancelled in OnStop: https://github.com/Southclaws/storyden/blob/4893fcca03b52ac2ac88926f3b3b53d7424fc627/internal/infrastructure/db/db.go#L25-L40

Also, I considered exposing a global context to depend on, not sure if it's idiomatic, but I discussed that here: https://github.com/uber-go/fx/discussions/905

screwyprof commented 1 year ago

Another typical example is a an open telemetry tracer

func NewExporter(ctx context.Context, endpoint string) (*otlptrace.Exporter, error) {
    traceOpts := []otlptracegrpc.Option{
        otlptracegrpc.WithTimeout(exporterConnTimeout),
        otlptracegrpc.WithInsecure(),
        otlptracegrpc.WithEndpoint(endpoint),
    }

    exporter, err := otlptracegrpc.New(ctx, traceOpts...)
    if err != nil {
        return nil, merry.Wrap(err)
    }

    return exporter, nil
}

func NewTracerProvider(
    ctx context.Context,
    res *resource.Resource,
    exporter *otlptrace.Exporter
) (TracerProvider, error) {
    tp := trace.NewTracerProvider(
        trace.WithSampler(trace.AlwaysSample()),
        trace.WithResource(res),
        trace.WithSpanProcessor(trace.NewBatchSpanProcessor(exporter)),
    )

    propagator := propagation.NewCompositeTextMapPropagator(
        propagation.TraceContext{},
        propagation.Baggage{},
    )

    otel.SetTracerProvider(tp)
    otel.SetTextMapPropagator(propagator)

    return tp, nil
}
abhinav commented 1 year ago

I don't work on the project anymore, but some historical context (heh): We were originally in favor of a context.Context associated with the lifetime of the fx.App available to the container. The idea was deferred until there was a demand for it.

At this point in time we should solve this but not by adding a context.Context to the container. This will break users who have added their own contexts to the container—similar to @Southclaws.

Fx friends, consider a new type with a Context() method that returns the fx.App-scoped context and provide that to all containers. Strawman suggestion:

// RunningApp is provided to all running Fx applications.
// It gives access to information about the current application.
type RunningApp struct{ /* ... */ }

// Context returns a context.Context that is valid as long as the App is running.
// The returned context invalidates itself when the App shuts down.
func (*RunningApp) Context() context.Context

I don't like the name RunningApp much. If you can think of something better, I recommend using that.

I would also suggest avoiding any API where you declare a new type Context that implements or wraps context.Context. That bears risk of someone trying to pass fx.Context down to their application code instead of context.Context.

screwyprof commented 1 year ago

HI @abhinav. Thank you for your reply. Correct me if I'm wrong, but iirc, uber/fx does not expose its context. If I get it correctly your suggestion was to do something along the lines:

// A couple of problems here:
// - app doesn't expose its context
// - even if the context is exposed, the app hasn't been created yet
ctx := ?

ra := &RunningApp{ctx: ctx}

app :=  fx.New(fx.Provide(func() *RunningApp{ return ra }))
app.Run()

If we simply created our own context, we would have to also listen for signals to be able to gracefully shutdown. Let me know if I'm missing anything.

JacobOaks commented 1 year ago

Hey @screwyprof, I think @abhinav is actually suggesting (correct me if I'm wrong) a modification to Fx where Fx will provide the RunningApp (name WIP) type for you, similar to how it does for Shutdowner or Lifecycle. All you'd have to do to get the app-scoped context is depend on it in your function:

app := fx.New(fx.Provide(func(ra *fx.RunningApp) (thing, error) {
     ctx := ra.Context()
    // do stuff with ctx
})
screwyprof commented 1 year ago

Hey @JacobOaks thanks for elaboration. Now it does make sense to me.

abhinav commented 1 year ago

Yep, @JacobOaks is correct. I'm suggesting adding this to Fx natively.