OrchardCore: Adding HttpClient Service out of order causes NullReferenceException

Hello,

We were originally experiencing this with Orchard RC1, but due to the stack trace I suspected it may be an dot net 3.0.x issue, however after upgrading to RC2 and dotnet 3.1.5, the same behavior is happening.

When we enable the OrchardCore.Recaptcha module, an error page is presented showing the following stack trace:

System.NullReferenceException: Object reference not set to an instance of an object.
   at Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.ReserveClient(IHttpClientBuilder builder, Type type, String name, Boolean validateSingleType)
   at Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.AddTypedClientCore[TClient](IHttpClientBuilder builder, Boolean validateSingleType)
   at Microsoft.Extensions.DependencyInjection.HttpClientFactoryServiceCollectionExtensions.AddHttpClient[TClient](IServiceCollection services)
   at OrchardCore.ReCaptcha.Core.ServiceCollectionExtensions.AddReCaptcha(IServiceCollection services, Action`1 configure) in C:\projects\orchardcore\src\OrchardCore\OrchardCore.ReCaptcha.Core\ServiceCollectionExtensions.cs:line 15
   at OrchardCore.ReCaptcha.Startup.ConfigureServices(IServiceCollection services) in C:\projects\orchardcore\src\OrchardCore.Modules\OrchardCore.ReCaptcha\Startup.cs:line 19
   at OrchardCore.Environment.Shell.Builders.ShellContainerFactory.CreateContainer(ShellSettings settings, ShellBlueprint blueprint) in C:\projects\orchardcore\src\OrchardCore\OrchardCore\Shell\Builders\ShellContainerFactory.cs:line 155
   at OrchardCore.Environment.Shell.Builders.ShellContextFactory.CreateDescribedContextAsync(ShellSettings settings, ShellDescriptor shellDescriptor) in C:\projects\orchardcore\src\OrchardCore\OrchardCore\Shell\Builders\ShellContextFactory.cs:line 76
   at OrchardCore.Environment.Shell.Builders.ShellContextFactory.OrchardCore.Environment.Shell.Builders.IShellContextFactory.CreateShellContextAsync(ShellSettings settings) in C:\projects\orchardcore\src\OrchardCore\OrchardCore\Shell\Builders\ShellContextFactory.cs:line 48
   at OrchardCore.Environment.Shell.ShellHost.GetOrCreateShellContextAsync(ShellSettings settings) in C:\projects\orchardcore\src\OrchardCore\OrchardCore\Shell\ShellHost.cs:line 87
   at OrchardCore.Environment.Shell.ShellHost.GetScopeAsync(ShellSettings settings) in C:\projects\orchardcore\src\OrchardCore\OrchardCore\Shell\ShellHost.cs:line 118
   at OrchardCore.Modules.ModularTenantContainerMiddleware.Invoke(HttpContext httpContext) in C:\projects\orchardcore\src\OrchardCore\OrchardCore\Modules\ModularTenantContainerMiddleware.cs:line 49
   at tusdotnet.TusCoreMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.HttpOverrides.HttpMethodOverrideMiddleware.Invoke(HttpContext context)
   at Microsoft.AspNetCore.Diagnostics.DeveloperExceptionPageMiddleware.Invoke(HttpContext context)

After much digging, this issue is caused by a combination of two things.

First, in the Microsoft code for AddHttpClient it registers a HttpClientMappingRegistry which must be an instance,

https://github.com/dotnet/extensions/blob/be18161fbed286d6fb7a7b1c7999ce70a50cf3eb/src/HttpClientFactory/Http/src/DependencyInjection/HttpClientFactoryServiceCollectionExtensions.cs#L51-L53

However in the Orchard code, this service gets converted into a factory method, because HttpClientMappingRegistry does not implement IDisposable:

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

This causes the subsequent AddHttpClient calls to fail with an NRE because the Microsoft code is not expecting a service descriptor with a factory method.

If all HttpClients are registered either in the Orchard Module Startup files or in the AddOrchardCms().ConfigureServices(), then things will work as expected. But, there are many third party packages that register HttpClients without the knowledge of the developer (such as health checks) so this work around isn’t really suitable long term.

About this issue

Original URL
State: closed
Created 4 years ago
Comments: 22 (9 by maintainers)

Commits related to this issue

Fixes #6677 duplicate startup — committed to OrchardCMS/OrchardCore by jtkech 4 years ago

Most upvoted comments

@marlon-tucker

You are totally right, your startup is first found by reflexion, because the app is a module and we lookup to any public type of the assembly, here the app assembly, and then registered in the tenant container, and the registration made at the app level through RegisterStartup is also found.

It works for the startup defined in OC.Mvc.Core that is not a module, because it is not defined in the app assembly and we don’t lookup for types defined in projects / packages referenced by the module, only the module assembly (here the app).

So, RegisterStartup should only be used to register a StartupBase that is not defined in a module, including the app that behaves as a module, e.g. as we use it for the startup of OC.Mvc.Core.

Not sure we need to fix something here, maybe just add it to the docs, otherwise we need to update RegisterStartup to not register a module (including the app assembly) startup, or in the 2 places there are resolved only use distinct types.

Update: But easy to fix by just doing this

moduleServiceCollection.TryAddEnumerable(ServiceDescriptor.Singleton(typeof(IStartup), dependency.Key));
tenantServiceCollection.TryAddEnumerable(ServiceDescriptor.Singleton(typeof(IStartup), dependency.Key));

In place of this

moduleServiceCollection.AddSingleton(typeof(IStartup), dependency.Key);
tenantServiceCollection.AddSingleton(typeof(IStartup), dependency.Key);

I think i will do a PR

jtkech on Jul 17, 2020

@marlon-tucker from the look of your startup you are adding a lot of services at application host level, rather than at tenant services / modular level.

One of those maybe overriding something that .AddOrchardCms() is doing

I would suggest trying to disable much of your startup and work backwards.

To register things at a tenant services level you can use AddOrchardCms().ConfigureServices() extensions.

deanmarcussen on Jul 15, 2020