Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Loss of scoped lifetime service instances when resolving HttpClient services configured through IServiceCollection.AddHttpClient(...) #42142

Closed
1 of 3 tasks
BoricuaEnLaLuna opened this issue Dec 27, 2019 · 18 comments

Comments

@BoricuaEnLaLuna
Copy link

Issue description

Hi, it seems there is an issue that causes scoped lifetime service instances to be lost (and potentially re-instantiated within the same request/scope). This is completely the opposite of what one would intend when setting up a service with scoped lifetime and can cause bugs that are really cryptic to figure out (especially since the behavior can vary depending on the order in which services are defined in the constructors of the services they are injected into).

This seems to happen only when resolving services that are setup as "http clients" following the instructions in the following documentation: https://docs.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-2.2

This is a bit tedious to explain, but Ill try my best here. I am using Azure Functions V2 (.Net Core 2.2 & Microsoft.Extensions.Http 2.2.0). For the purposes of the following scenarios assume I have the following:

    public class Functions
    {
        private readonly IScoped1 scoped1;
        public Functions(IScoped1 scoped1)
        {
            this.scoped1 = scoped1;
        }
        [FunctionName("Function1")]
        public async Task<IActionResult> Run(
            [HttpTrigger(AuthorizationLevel.Anonymous, "get", "post", Route = null)] HttpRequest req,
            ILogger log)
        {
            log.LogInformation("C# HTTP trigger function processed a request.");
            return new OkObjectResult($"Hello");
        }
    }
    public interface IScoped1 { }
    public class Scoped1 : IScoped1
    {
        private readonly IScoped2 scoped2;
        private readonly IHttpClient1 httpClient1;
        private readonly IHttpClient2 httpClient2;
        public Scoped1(IScoped2 scoped2, IHttpClient1 httpClient1, IHttpClient2 httpClient2)
        {
            Console.WriteLine($"{this.GetType().Name}()");
            this.scoped2 = scoped2;
            this.httpClient1 = httpClient1;
            this.httpClient2 = httpClient2;
        }
    }

    public interface IScoped2 { }
    public class Scoped2 : IScoped2
    {
        public Scoped2()
        {
            Console.WriteLine($"{this.GetType().Name}()");
        }
    }

    public interface IHttpClient1 { }
    public class HttpClient1 : IHttpClient1
    {
        private readonly HttpClient httpClient;
        private readonly IScoped2 scoped2;
        public HttpClient1(HttpClient httpClient, IScoped2 scoped2)
        {
            Console.WriteLine($"{this.GetType().Name}()");
            this.httpClient = httpClient;
            this.scoped2 = scoped2;
        }
    }

    public interface IHttpClient2 { }
    public class HttpClient2 : IHttpClient2
    {
        private readonly HttpClient httpClient;
        private readonly IScoped2 scoped2;
        public HttpClient2(HttpClient httpClient, IScoped2 scoped2)
        {
            Console.WriteLine($"{this.GetType().Name}()");
            this.httpClient = httpClient;
            this.scoped2 = scoped2;
        }
    }

Scenario 1: If NOT using AddHttpClient the scoped lifetimes work as expected.

In this case I setup my http client services as simple Transients:

            builder.Services.AddScoped<IScoped1, Scoped1>();
            builder.Services.AddScoped<IScoped2, Scoped2>();
            builder.Services.AddTransient<IHttpClient1, HttpClient1>();
            builder.Services.AddTransient<IHttpClient2, HttpClient2>();

With this setup when I call the "Function1" httptrigger through POSTMAN I see the expected sequence of service instantiations:

  1. Scoped2()
  2. HttpClient1()
  3. HttpClient2()
  4. Scoped1()

Scenario 2: If using AddHttpClient then scoped lifetime services can be instantiated more than once per request.

In this case I setup my http client services with AddHttpClient following the documentation I referenced above:

            builder.Services.AddScoped<IScoped1, Scoped1>();
            builder.Services.AddScoped<IScoped2, Scoped2>();
            builder.Services.AddHttpClient<IHttpClient1, HttpClient1>();
            builder.Services.AddHttpClient<IHttpClient2, HttpClient2>();

With this setup when I call the "Function1" httptrigger through POSTMAN I see the unexpected re-instantiations of class Scoped2:

  1. Scoped2()
  2. Scoped2() --> This should be reusing the previous instantation of Scoped2
  3. HttpClient1()
  4. Scoped2() --> This should be reusing the previous instantation of Scoped2
  5. HttpClient2()
  6. Scoped1()

Additional to this example if I switch around the order of the parameters in the constructor of class "Scoped1" I also see different unexpected instantiation sequences. After playing around with this for a while I believe what is happening is that each time an "http client" service is resolved for the very first time in the lifetime of a request, it seems to clear any services that may have already been resolved in the scoped lifetime up to that point. However if that same service is resolved a 2nd+ time (within the same request lifetime) then it no longer seems to clear the scoped lifetime services.

Is this a known issue or am I misunderstanding something?
Thanks

Software versions

Check the .NET target framework(s) being used, and include the version number(s).

  • .NET Core (v2.2) through Azure Functions V2
  • .NET Framework
  • .NET Standard

If using the .NET Core SDK, include dotnet --info output. If using .NET Framework without the .NET Core SDK, include info from Visual Studio's Help > About Microsoft Visual Studio dialog.

dotnet --info output or About VS info
<replace>
@Rick-Anderson
Copy link
Contributor

@pranavkm @mkArtakMSFT can one of you transfer this if appropriate? If no, please respond.

@BoricuaEnLaLuna
Copy link
Author

BoricuaEnLaLuna commented Jan 2, 2020

After further experimentation on this I believe I have narrowed down the scenario where this issue surfaces. Anytime the HttpClientFactory internally determines that it needs to generate a new HttpClientMessageHandler it seemingly wipes out whatever scoped lifetime service instances were already instantiated at the time. Otherwise if the HttpClientFactory creates a new instance of HttpClient but without creating new HttpClientMessageHandlers then the scoped lifetime services remain intact.

This would explain why on the first resolution of HttpClients in my sample code above it forces re-instantiation of class Scoped2 (since on the first time it must create message handlers) and on the 2nd+ resolutions it doesn't... that is until some time later when the HttpClientFactory determines it must create new MessageHandlers again. At that point the scoped services are wiped out again.

@mkArtakMSFT
Copy link
Member

Thanks for the details, @BoricuaEnLaLuna.
@rynowak should this be moved over to aspnetcore repo and be tracked as an Issue instead?

@mkArtakMSFT
Copy link
Member

Ping @rynowak

@tunestrivia
Copy link

@rynowak have you had a chance to confirm this behavior?

Thanks!

@BoricuaEnLaLuna
Copy link
Author

I believe I provided all the needed details. If the behavior is indeed as I described it blocks me from using HttpClient services since I will not have control over this behavior and it causes random failures for my solution. I would appreciate if there could be some updates here since it has been a while now.

Thanks!

@mkArtakMSFT mkArtakMSFT transferred this issue from dotnet/AspNetCore.Docs May 15, 2020
@BoricuaEnLaLuna
Copy link
Author

Hi folks, any chance there are any updates on this? Has someone at least been able to confirm the behavior I described above 8 months ago?

Thanks!

@BoricuaEnLaLuna
Copy link
Author

Hi again, I had put this project on hold ever since I found & opened this issue but decided to revisit it this past weekend. I can confirm again this is still very much reproable. I also tried updating to the latest versions of all the relevant libraries (including upgrade to Azure Functions v3) and the same issue also easily reproes there.

For your convenience I have attached my own version of the sample projects (one with my original Azure Functions V2 setup and a second one with latest Azure Functions V3). All you need to do is:

  1. Run either of these projects
  2. Hit the http://localhost:7071/api/Function1 endpoint (with POSTMAN, Fiddler, etc.)
  3. Look at the output Debug traces to see the behavior I described originally.
  4. If you want to compare the behavior without use of AddHttpClient then just replace the value of the following variable in Startup.cs: private const bool UseAddHttpClient = true;

@Pilchie could you please tag whoever is needed for investigation on this? If my outside-view assessment of the issue is correct I feel this merits some attention.

AddHttpClient.zip
.

Thanks!

@Pilchie
Copy link
Member

Pilchie commented Aug 10, 2020

@karelz any ideas who could take a look here?

@BoricuaEnLaLuna sorry for the slowness here. Unfortunately, the primary engineer who worked on HttpClientFactory is no longer on the team, so it might take a bit to dig in here.

@karelz
Copy link
Member

karelz commented Aug 10, 2020

I can assign someone next week-ish. Unfortunately we have to ramp up on the code base and API usage patterns from scratch first :(, so it will take some time.

@BoricuaEnLaLuna
Copy link
Author

I understand if it takes some time for ramp-up.
Thanks, appreciate it!

@BrennanConroy BrennanConroy transferred this issue from dotnet/aspnetcore Sep 11, 2020
@jeffschwMSFT jeffschwMSFT added untriaged New issue has not been triaged by the area owner area-System.Net.Http labels Sep 12, 2020
@ghost
Copy link

ghost commented Sep 12, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@ghost
Copy link

ghost commented Sep 13, 2020

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

@ericstj
Copy link
Member

ericstj commented Sep 14, 2020

Ping @dotnet/ncl @karelz

@ericstj ericstj removed the untriaged New issue has not been triaged by the area owner label Sep 16, 2020
@ericstj ericstj added this to the 6.0.0 milestone Sep 16, 2020
@DillonN
Copy link

DillonN commented Dec 3, 2020

Maybe another perspective would help, if you consider this service registration:

services.AddHttpClient<MyClient>(ConfigureClient).AddHttpMessageHandler(CreateHandler);

DelegatingHandler CreateHandler(IServiceProvider sp)
{
    // Something where scope of sp is important
}

void ConfigureClient(HttpClient client) { ... }

Since MyClient is registered as transient, I would expect this to work. I.e. I would expect the scope of sp in CreateHandler to be the same as where MyClient is being resolved from

But this doesn't work because DefaultHttpClientFactory is managing the IServiceProvider and it's a singleton:

IServiceProvider services = _services;
var scope = (IServiceScope)null;
HttpClientFactoryOptions options = _optionsMonitor.Get(name);
if (!options.SuppressHandlerScope)
{
scope = _scopeFactory.CreateScope();
services = scope.ServiceProvider;
}
try
{
HttpMessageHandlerBuilder builder = services.GetRequiredService<HttpMessageHandlerBuilder>();
builder.Name = name;

So instead you're forced to workaround by basically replacing AddHttpClient<T> and DefaultHttpClientFactory.CreateClient, something along the lines of:

services.AddHttpClient();
services.AddTransient<MyClient>(sp =>
{
    var messageHandlerFactory = sp.GetRequiredService<IHttpMessageHandlerFactory>();
    var clientFactory = sp.GetRequiredService<ITypedHttpClientFactory<MyClient>>();

    var factoryHandler = messageHandlerFactory.CreateHandler();
    var fullHandler = CreateHandler(sp);
    fullHandler.InnerHandler = factoryHandler;

    var httpClient = new HttpClient(fullHandler, disposeHandler: false);
    ConfigureClient(httpClient);

    return clientFactory.CreateClient(httpClient);
}

Which is not ideal for many reasons.

I see a couple possible solutions for this, but unfortunately nothing non-breaking.. One way though could be to add an overload to IHttpClientFactory.CreateClient(string) that also takes an IServiceProvider so one could be passed down. Another would be registering DefaultHttpClientFactory as transient and moving the pooling stuff to a separate singleton service. Again not ideal

But then you get into a more fundamental problem. Since the main point of DefaultHttpClientFactory is to pool and reuse handlers across scopes, having them always be built from the resolving scope definitely can't be guaranteed. Maybe that's when relying on the SocketsHttpHandler feature makes a lot of sense (#35987)

Edit: there's some other great discussion on this in #43660

@CarnaViire
Copy link
Member

Triage: Reproduces on Azure Functions environment, doesn't reproduce on .NET Core Generic Host. Next step: try ASP.NET Core environment

@CarnaViire CarnaViire removed this from the 6.0.0 milestone Feb 1, 2021
@CarnaViire CarnaViire added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Feb 1, 2021
@CarnaViire
Copy link
Member

CarnaViire commented Feb 2, 2021

The issue was also not reproducible on ASP.NET Core which made me search for the root of the problem inside Azure Functions environment.

I discovered that by default Azure Functions work with DI scopes in a non-"conventional" way: creating a new scope "hides" an existing scope until that new scope is disposed. That is why the behavior described in this issue is only reproducible in Azure Functions environment. HttpClientFactory implementation relies on the fact that any number of scopes can exist and not interfere with each other, and that is true for both ASP.NET and .NET Generic Host.

There is actually a tracking issue for that Azure/azure-functions-host#5098
In this issue, it is advised to use feature flag if you want to get a "conventional" behavior for scopes:
"AzureWebJobsFeatureFlags": "EnableEnhancedScopes"
Adding this flag fixed the issue for me.

Please let me know if adding this feature flag solved the problem @BoricuaEnLaLuna

@CarnaViire CarnaViire removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Feb 2, 2021
@CarnaViire
Copy link
Member

Closing as answered above

@CarnaViire CarnaViire added this to the 6.0.0 milestone Feb 4, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Mar 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests