OrchardCore: `AddHttpClient()` fails within Modules.

new version of #6677 as it was asked that a new issue be created.

Typed calls to AddHttpClient are failing within a module in .Net 5.0 and .Net 6.0. (I tried rolling back to .Net 5.0 in case OC 1.1 was incompatibile with 6.0).

// MainProject/Startup.cs
class MainStartup
{        
  public void ConfigureServices(IServiceCollection services)
  {
    services.AddHttpClient<MyService>(); // this is cool
  }
}

// Module/Startup.cs
class ModuleStartup : OrchardCore.Modules.StartupBase
{ 
  public void ConfigureServices(IServiceCollection services)
  {
    services.AddHttpClient<MyService>(); // NullReferenceException
  }
}

The issue is as follows. In AspNetCore, AddHttpClient registers a singleton of HttpClientMappingRegistry.

https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs#L52-L54

When initializing a typed HTTP Client, HttpClientBuilderExtensions.AddTypedClient calls HttpClientBuilderExtensions.ReserveClient, which iterates through the Builder’s Services and attempts to find an instantiated instance of HttpClientMappingRegistry.

https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientBuilderExtensions.cs#L634-L636

Orchard Core’s FeatureAwareServiceCollection doesn’t automatically instantiate Singletons, for the reason that @marlon-tucker linked in the initial issue – it doesn’t implement IDisposable, so it isn’t instantiated until ServiceProvider.GetService<HttpClientMappingRegistry>() is called:

https://github.com/OrchardCMS/OrchardCore/blob/ec2154ae099913ee81751f30c4f7660952fe63c9/src/OrchardCore/OrchardCore/Shell/Builders/Extensions/ServiceProviderExtensions.cs#L42-L59

As you can see in the ReserveClient method above, however, GetService is never called on the HttpClientMappingRegistry – it is instead selected through LINQ, meaning it is never instantiated. Single() returns a registered service, but Single().ImplementedInstance is null.

I’ll try and provide a repro when I have the chance.

About this issue

Original URL
State: closed
Created 2 years ago
Reactions: 3
Comments: 39 (33 by maintainers)

Commits related to this issue

Initial commit for https://github.com/OrchardCMS/OrchardCore/issues/11041 — committed to johnrom/orchard-core-issue-reproduction by johnrom 2 years ago

Most upvoted comments

We have a main TenantStartup.cs that we run before doing any other modules and were able to work around this problem by adding the following code earlier in that

    var registry = services.SingleOrDefault(sd => sd.ServiceType.Name == "HttpClientMappingRegistry");
                if (registry?.ImplementationInstance == null)
                    services.Remove(registry);

ShaneCourtrille on Mar 14, 2023

In the meantime here some thoughts based on what I saw.

First I think that connections sharing is done at a lower IO level.
What I saw is that anyway a new HttpClient is always created and that handlers are resolved or activated, for a typed client an object factory is cached but not sure it takes a lot of memory.

Yes, this is the Handlers Pipeline that is cached when built by the DefaultHttpClientFactory, but here too not sure that it takes a lot of memory, this is not the activated handler instances that are cached.

Note: Tenants are isolated “Sites” with their Filters/Handlers, Pipeline, APIs and so on, so make sense that they also have their own http client Handlers Pipeline, but I understand the memory concern.
One annoying thing is that the client factory creates a scope that will be used to activate handlers, which is not a ShellScope which e.g. prevents a shell to be disposed while a scope is still active.
```
      if (!options.SuppressHandlerScope)
      {
          scope = _scopeFactory.CreateScope();
          services = scope.ServiceProvider;
      }
```
Anyway the scope is created from the service provider resolved by the factory, so what happens if a DelegatingHandler resolve services that may depend on the enabled features of a given tenant.

This is the case of our Twitter handler that injects tenant specific services including a security service, more over this handler is only registered by the tenants having the Twitter feature enabled.

Note: Here we may need a custom service provider as we use for the RequestServices that we made aware of the current ShellScope so that it returns the ShellScope.Services if a current one exists.
So, in case there is not so much memory consumption by duplicating some caches, maybe a good compromise would be to support the point 2. from my previous list.

This to allow http client configurations at both host and tenant levels but without sharing them, and let it working as before at the tenant level (no sharing across tenants).

jtkech on Jun 1, 2023

Thanks for the detailed repro @johnrom

I suspect from reading the code that the issue exists because you are registering AddHttpClient() at both a module level and the main host startup module. Which is one way I can see the code you linked failing.

Does AddHttpClient() work fine from a module, when you do not register it at app level?

I’m guessing it does, because we (OC) use AddHttpClient() in a few of our modules already, Twitter, Recaptcha, and personally I use it in modules quite regularly.

Can you confirm?

Should make it easier to understand, what we can (if anything) do about it.

deanmarcussen on Jan 20, 2022