fx icon indicating copy to clipboard operation
fx copied to clipboard

Using context when providing a new dependency

Open screwyprof opened this issue 2 years ago • 7 comments

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
}

screwyprof avatar May 06 '22 17:05 screwyprof

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

Southclaws avatar Sep 06 '22 11:09 Southclaws

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
}

screwyprof avatar May 24 '23 17:05 screwyprof

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.

abhinav avatar May 24 '23 23:05 abhinav

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.

screwyprof avatar May 26 '23 13:05 screwyprof

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

JacobOaks avatar May 26 '23 13:05 JacobOaks

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

screwyprof avatar May 26 '23 14:05 screwyprof

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

abhinav avatar May 26 '23 16:05 abhinav