RFC: Add ServiceProviderAccessor to allow access to IServiceProvider from Interceptor

[tdg5]

What was changed

This PR is a sketch of a solution for #363 that leverages a pattern that's used in ASP.NET for managing access to the HttpContext instance that is appropriate for a given execution context.

I don't think this PR is done, but I wanted to sketch something for discussion before putting more effort in.

Some things to note:

• The default execution path is unchanged. Alternate code paths are only executed if a given application registers an IServiceProviderAccessor instance with the IServiceCollection.
• This is still convoluted, but it does make accessing the scoped IServiceProvider from an ActivityInboundInterceptor possible without ActivityInboundInterceptor needing to know anything about IServiceProvider.

Why?

I want to be able to use the scoped IServiceProvider instance used to construct the current Activity from an ActivityInboundInterceptor that is dealing with a cross-cutting concern that would benefit from access to the DI service provider.

For example, I have an ActivityInboundInterceptor, call it UserInfoInterceptor that interrogates the Activity to figure out what user work is being performed for and then makes that information available to other DI services that care about that context using a service registered with the IServiceProvider. I'd now like to add a new ActivityInboundInterceptor, call it LogDecoratorInterceptor, that decorates the log context with details about the user. This change would allow me to use the mechanism that UserInfoInterceptor provides via IServiceProvider instead of having to recompute the user or do other black magic.

Checklist

1. Closes #363

2. How was this tested:
   Not tested, just a proposal (though I do expect that it works)

3. Any docs updates needed?
   I would think an example would be needed because even with this accessor, things are more convoluted than might be ideal. Consider:

During the build phase of the application, it would be necessary to create an instance of ServiceProviderAccessor and provide it to any IWorkerInterceptor that cares about accessing the scoped IServiceProvider instance later. The code would then need to register the ServiceProviderAccessor instance with the ServiceCollection being constructed as a Singleton so that ServiceProviderExtensions.CreateTemporalActivityDefinition could fetch the ServiceProviderAccessor instance later to register the scoped IServiceProvider instance.

To try to put that in code, here's a hypothetical Program.cs

using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Temporalio.Extensions.Hosting;
using Temporalio.Worker.Interceptors;

namespace Temporalio.Issue363Demo;

internal class Program
{
    private static async Task Main(string[] args)
    {
        HostApplicationBuilder builder = Host.CreateApplicationBuilder(args);

        var temporalWorkerBuilder = builder.Services.AddHostedTemporalWorker(
            buildId: null, taskQueue: "TASK_QUEUE");

        ServiceProviderAccessor serviceProviderAccessor = new();
        MyInterceptor myInterceptor = new(serviceProviderAccessor);
        builder.Services.AddSingleton<IServiceProviderAccessor>(serviceProviderAccessor);

        temporalWorkerBuilder.ConfigureOptions(options =>
        {
            options.Interceptors = [myInterceptor];
        });

        var host = builder.Build();
        // ...
    }
}

public class MyInterceptor : IWorkerInterceptor
{
    private readonly IServiceProviderAccessor serviceProviderAccessor;

    public MyInterceptor(IServiceProviderAccessor serviceProviderAccessor)
    {
        this.serviceProviderAccessor = serviceProviderAccessor;
    }

    public ActivityInboundInterceptor InterceptActivity(
        ActivityInboundInterceptor next) =>
            new MyActivityInboundInterceptor(serviceProviderAccessor, next);
}

public class MyActivityInboundInterceptor : ActivityInboundInterceptor
{
    private readonly IServiceProviderAccessor serviceProviderAccessor;

    public MyActivityInboundInterceptor(IServiceProviderAccessor serviceProviderAccessor,
        ActivityInboundInterceptor next)
        : base(next)
    {
        this.serviceProviderAccessor = serviceProviderAccessor;
    }

    public override Task<object?> ExecuteActivityAsync(ExecuteActivityInput input)
    {
        var serviceProvider = serviceProviderAccessor.ServiceProvider;
        // Do something with serviceProviderAccessor!
        return Task.FromResult<object?>("yay!");
    }
}

[CLAassistant]

All committers have signed the CLA.

[tdg5]

Adapted from ASP.NET's IHttpContextAccessor.

[tdg5]

Adapted from ASP.NET's HttpContextAccessor.

[tdg5]

Inspiration taken from ASP.NET's DefaultHttpContextFactory.

[tdg5]

A System.Collections.Concurrent.ConcurrentDictionary that is keyed on the Activity's unique ID could be a simpler replacement for this and would work just as well since it would depend on the AsyncLocal holding the current Activity.

[cretz]

Thanks! Hrmm, I am unsure we want to provide an async local form of what users can provide themselves. .NET expects, if you want access to the service provider, you inject it like anything else. Why not just accept service provider in the activity class constructor if you need it? Something like:

public class MyActivities
{
    private readonly IServiceProvider serviceProvider;
    private readonly IWhateverElse whateverElse;

    public MyActivities(IServiceProvider serviceProvider, IWhateverElse whateverElse)
    {
        this.serviceProvider = serviceProvider;
        this.whateverElse = whateverElse;
    }

    [Activity]
    public string DoMyActivity(string someParam)
    {
        // serviceProvider available here
        throw new NotImplementedException("TODO");
    }
} 

This way IServiceProvider is not special, it's just injected like anything else might be. If you as a user wanted to put some injected thing (like service provider) into an async local, you can.

[tdg5]

@cretz, thanks for the suggestion, but how do I that with an interceptor?

The issue I'm trying to find a solution for relates to handling a cross-cutting concern that would need to be handled in every Activity.

For example, consider adding support for OpenTelemetry in every Activity compared to adding support for OpenTelemetry via an interceptor that allows each Activity to be agnostic of OpenTelemetry and to instead focus on its actual job.

Support for OpenTelemetry avoids needing DI because diagnostic activities don't require a reference to the specific trace provider. But if that weren't the case, if supporting OpenTelemetry required the interceptor to have a reference to the specific trace provider and that trace provider were only accessible via the service provider, how would support for OpenTelemetry be handled other than by adding logic to every Activity to handle OpenTelemetry concerns via the injected IServiceProvider? It would no longer be a problem that could be solved with an interceptor.

Put another way, if there's some common, scoped service registered with the IServiceProvider, why should I have to add logic to every Activity in order to handle setup/teardown with that scoped service?

Put yet another way, if I were making a library for use with Temporal, how could I provide a library that depended on a scoped service from IServiceProvider and didn't need to be explicitly called in every Activity?

[cretz]

thanks for the suggestion, but how do I that with an interceptor?

An interceptor cannot control what an activity implementer chooses to dependency inject. This same concern occurs with anything to dependency inject not just service provider.

Support for OpenTelemetry avoids needing DI because diagnostic activities don't require a reference to the specific trace provider. But if that weren't the case, if supporting OpenTelemetry required the interceptor to have a reference to the specific trace provider and that trace provider were only accessible via the service provider, how would support for OpenTelemetry be handled other than by adding logic to every Activity to handle OpenTelemetry concerns via the injected IServiceProvider?

DI is optional in .NET and I believe the reason that OpenTelemetry and .NET diagnostic activities use a shared tracer provider and shared .NET diagnostic activity sources is because the per-DI-scope design is too limiting for general purpose use. I wonder if your use case could not rely on optional DI and scoping the same way OTel does not.

why should I have to add logic to every Activity in order to handle setup/teardown with that scoped service?

Because it is a specific need in your case to have implicit access to the DI container in a non-DI instantiated interceptor. One approach may be leveraging IServiceScopeFactory to set your async local. For example, maybe something like:

public class MyServiceScopeFactory : IServiceScopeFactory
{
    public static readonly AsyncLocal<IServiceProvider> CurrentServiceProvider = new();

    private readonly IServiceScopeFactory underlying;

    public MyServiceScopeFactory(IServiceScopeFactory underlying) => this.underlying = underlying;

    public IServiceScope CreateScope()
    {
        var scope = underlying.CreateScope();
        CurrentServiceProvider.Value = scope.ServiceProvider;
        return scope;
    }
}

And change host builder to have something like .ConfigureServices(services => services.AddSingleton<IServiceScopeFactory>(provider => new MyServiceScopeFactory(provider.GetRequiredService<IServiceScopeFactory>())) but I have not tested it.

The other option we'd entertain is a way to make the activity inbound interceptor be created in the current scope. My concern is exposing a static based on async local if we don't have to, but maybe we can have some ITemporalWorkerServiceOptionsBuilder extension like .AddScopedActivityInterceptor<ActivityInboundInterceptor> that we make sure somehow to instantiate with the service provider. I suspect we'd have to use ActivatorUtilities.CreateInstance to be able to provide the next interceptor. Maybe it would be powered internally by async local (i.e. an internal interceptor with static async local that is set the way this PR does, but remains internal). I would have to sit down and design this a bit.

[tdg5]

Hey @cretz , thanks for the thorough response.

I like your IServiceScopeFactory idea and have explored it, but it doesn't seem to be supported.

I'm out of time for this today, but will respond to your other comments later this week. ❤️

[cretz]

Interesting. So I am now starting to lean towards providing the service provider in the interceptor only. Meaning I wonder if we should keep this async local internal and have something like:

namespace Temporalio.Extensions.Hosting;

interface IWorkerInterceptorWithServiceProvider : IWorkerInterceptor
{
    ActivityInboundInterceptor InterceptActivity(ActivityInboundInterceptor nextInterceptor) =>
        InterceptActivity(WhateverInternalAsyncLocal.Value, nextInterceptor);

    ActivityInboundInterceptor InterceptActivity(IServiceProvider serviceProvider, ActivityInboundInterceptor nextInterceptor) =>
        nextInterceptor;
}

My main concern is I am not wanting people to generally use an async local to access the scope or service provider inside the activity if we can help it. I agree that constructor injection doesn't work with interceptors, so maybe this can work around it this way (still using async local internally though).

[tdg5]

hey @cretz , sorry for not getting back to you. With the US holiday and some travel for work, I've been AFK for a few weeks.

I'd be fine if only the interceptor got the IServiceProvider: That's really the only gap I'm trying to find a solution for and I agree that such an implementation would be much cleaner than leaving it to users to fetch the IServiceProvider from the async local in broader, stranger contexts.

I think I'd confused myself on what library IServiceProvider comes from and was concerned that changing the interceptor would require making more code depend on Microsoft.Extensions.DependencyInjection, but it seems like IServiceProvider comes from System, so that shouldn't be a problem.

Is this something you'd like me to take a stab at or do you have a plan of attack in mind?

[tdg5]

Due to the need to access this across packages, this can't be internal unless Temporalio is modified to make internals available to Temporalio.Extensions.Hosting. I don't want this to be public, but need some guidance on the right way to hide this value from the public.

[tdg5]

There is also some tension here because an interface implementation of IWorkerInterceptorWithServiceProvider would actually require that this field (or a wrapping property) is public because applications that are not NETCOREAPP3_0_OR_GREATER would need to be able to access this field to replicate this logic

ActivityInboundInterceptor InterceptActivity(ActivityInboundInterceptor nextInterceptor) =>
    InterceptActivity(ActivityServiceProviderAccessor.Current, nextInterceptor);

[tdg5]

I feel like the interface approach to this is a bit clunky. Perhaps a class would be better?

[cretz]

Hrmm, now that I'm thinking about it, this may not work either. The interceptor is invoked before the service provider is created. The scope and service provider is created as part of the last interceptor in the chain. But you want a service provider before then. I'm now thinking of an alternative approach where you get a service provider in your interceptor and create a scope yourself if needed.

I now think you have to do something like this:

resultOfAddHostedTemporalWorkers.ConfigureOptions().PostConfigure<IServiceProvider>((options, provider) =>
{
    // Create an instance of the interceptor.
    // Could also do provider.GetRequiredService<MyInterceptor> if it was registered on the provider.
    var myInterceptor = new MyInterceptor(provider);

    // Add to interceptor list (extra code for checking if list exists may not be needed for you)
    var newInterceptors = new List<IWorkerInterceptor>();
    if (options.Interceptors is { } existing)
    {
        newInterceptors.AddRange(existing.Interceptors);
    }
    newInterceptors.Add(myInterceptor);
    worker.Interceptors = newInterceptors;
});

Then MyInterceptor can be something like:

public class MyInterceptor : IWorkerInterceptor
{
    private readonly IServiceProvider serviceProvider;

    public MyInterceptor(IServiceProvider serviceProvider) =>
        this.serviceProvider = serviceProvider;

    public ActivityInboundInterceptor InterceptActivity(ActivityInboundInterceptor next) =>
        new MyActivityInboundInterceptor(serviceProvider, next);
}

public class MyActivityInboundInterceptor : ActivityInboundInterceptor
{
    private readonly IServiceProvider serviceProvider;

    public MyActivityInboundInterceptor(IServiceProvider serviceProvider,
        ActivityInboundInterceptor next)
        : base(next) => this.serviceProvider = serviceProvider;

    public override Task<object?> ExecuteActivityAsync(ExecuteActivityInput input)
    {
        using scope = serviceProvider.CreateScope();
        // Do stuff with scope.ServiceProvider here...

        return await base.ExecuteActivityAsync(input)
    }
}

Is this acceptable? (untested, just typed in here)

[tdg5]

@cretz, I explored taking a stab at this (PR updated). My approach was to adapt my proposed ServiceProviderAccessor into Temporalio.Acitvities.ActivityServiceProviderAccessor that would be something like ActivityExecutionContext but responsible for the IServiceProvider instance.

Points of friction:

• Because ActivityServiceProviderAccessor is written from the Temporalio.Extensions.Hosting package and read from the Temporalio package, I couldn't make the AsyncLocalCurrent variable internal. I definitely want to hide AsyncLocalCurrent, but don't have any ideas other than exposing the internals of Temporalio to Temporalio.Extensions.Hosting. Wrapping access in a property doesn't help, IMHO. Thoughts?
• Implementing IWorkflowInterceptorWithServiceProvider as an interface feels kind of clunky given the work it leaves to apps that are not NETCOREAPP3_0_OR_GREATER. Maybe implementing an interface and including a base class implementation (e.g. WorkflowInterceptorWithServiceProvider) is a better option?

[cretz]

@tdg5 - take a peek at my suggestion above your comment. I don't think the IWorkerInterceptorWithServiceProvider will work, but there is a suggestion on how to get service provider in your interceptor without any code changes. Does it work for you?

[tdg5]

@cretz , just saw your comment. Catching up now.

[tdg5]

@cretz, IMHO, what you've suggested wouldn't work for what I'm trying to achieve. The same kind of thing could be achieved using a ServiceLocator kind of thing that is configured with the IServiceProvider instance after the application is started and then accessed by the Interceptor as necessary.

The trick is getting access to the specific scoped IServiceProvider that the activity code utilizes to do its work so the IWorkflowInterceptor implementation could utilize scoped services that are relevant to it but aren't necessarily relevant to the activity (or at least shouldn't be the concern of every activity implementation, e.g. cross cutting concerns).

A second scoped IServiceProvider could work okay in some cases, such as an when an Interceptor uses the ActivityContext to mutate some state that is global or related to the current execution context. But, I think it's more confusing to reason about and would double any work involved in reifying the scoped services (e.g. a DB query or web request to fetch relevant user information would need to occur for both scoped IServiceProvider instances).

I think allowing IWorkflowInterceptor and respective Activity instances to access the same scoped IServiceProvider is easier to reason about and enables better division of responsibilities and more powerful use cases that depend on collaborating with the injected services.

But all that said, it doesn't really seem like this can be achieved as currently implemented since the creation of the IServiceProvider happens at the end of the interceptor chain as a prelude to invoking the activity. Not sure how we both didn't catch that sooner. I'll keep brainstorming workarounds, but I'm not sure there's a way forward here.

[cretz]

Hrmm. To confirm, you need the exact same instance of scoped things in an activity that you have in the interceptor? You cannot use a separate outer scope for your interceptor needs? The interceptor is invoked out of scope. Yeah, if you must use interceptors and they must be in the same scope as the activity, we have to refactor because that is not the current case. Otherwise, out-of-scope interceptors may not bee acceptable for your in-scope needs.

If we can do it in a compatible way, I am ok instantiating the scope before the interceptor. The problem is that we intentionally don't give pre-activity-task/activation hooks to users (this separate library is a "user" in this sense). So we would now, in addition to having a concept of intercepting/wrapping activity work, need to have a concept of intercepting/wrapping the interception/wrapping of activity work. At some level we are always going to have to expose out-or-scope interception to external users so they do work like this DI library does create-scope.

I am open to designs here, though hopefully we can avoid another layer of wrapping/interception/hooks even higher than interceptors.