Orchardcore: Adding HttpClient Service out of order causes NullReferenceException

[marlon-tucker]

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.

[marlon-tucker]

I've tried this using the OrchardCore.Cms.Web project that's in the OrchardCore git repo using the RC2 tag and it works, so it must be something to do with the way our web project is initiated.

So the urgency on this has decreased as it isn't an issue with Orchard Core itself, but if anyone has any ideas on where I should be looking I would be grateful.

New info, see https://github.com/OrchardCMS/OrchardCore/issues/6677#issuecomment-658881058

[hishamco]

I've tried this using the OrchardCore.Cms.Web project that's in the OrchardCore git repo using the RC2 tag and it works, so it must be something to do with the way our web project is initiated.

Do you mean that when you enable ReCaptcha module from your app - which I supposed used the latest bits - it throws NRE?

[marlon-tucker]

Do you mean that when you enable ReCaptcha module from your app - which I supposed used the latest bits - it throws NRE?

Yeah, our app references the RC2 Nugets

[hishamco]

How your Startup is look like?

[marlon-tucker]

The line that is going pop is this:

SourceLinked from HttpClientBuilderExtensions.cs

var registry = (HttpClientMappingRegistry)builder.Services.Single(sd => sd.ServiceType == typeof(HttpClientMappingRegistry)).ImplementationInstance;

I'm still not sure where the NRE is originating from, as the Single() call should throw a InvalidOperationException if no items meet the given predicate.

I notice builder.Services is a Orchard type of OrchardCore.Environment.Shell.Builders.FeatureAwareServiceCollection

Our StartUp file is quite large unfortunately, here is our ConfigureServices section as that's what seems to be related to this.

        public void ConfigureServices(IServiceCollection services)
        {
            //services.AddMiddlewareAnalysis();

            services.AddAppConfig<WebAppConfig>(_rootConfig, out var initialConfig);
            Configuration = initialConfig;

            services.AddHttpContextAccessor();
            services.AddSingleton<ICookieAuthenticationOptionsProvider>(new CookieAuthenticationOptionsProvider());

            var maxReqyestBodyLength = Configuration.Security?.MaxRequestBodySizeBytes ?? AareonUK360.Common.Mvc.Constants.FirstTouchFormRequestBodySizeLimitBytes;

            services.Configure<IISServerOptions>(x =>
            {
                x.MaxRequestBodySize = maxReqyestBodyLength;
            });

            services.Configure<KestrelServerOptions>(x =>
            {
                x.Limits.MaxRequestBodySize = maxReqyestBodyLength;
                x.AddServerHeader = false;
            });

            services.Configure<FormOptions>(x =>
            {
                x.ValueCountLimit = 8192;
                x.MultipartBodyLengthLimit = maxReqyestBodyLength;
            });

            services.AddOrchardCms();

            services.Configure<MvcOptions>(options => options.Filters.Add<RefreshExpiringAccessTokenFilter>());

            services.AddDistributedMemoryCache();
            services.AddMemoryCache();

            services.AddSession(options =>
            {
                options.Cookie.HttpOnly = true;
                options.Cookie.SecurePolicy = CookieSecurePolicy.SameAsRequest;
            });

            services.AddAntiforgery(opts =>
            {
                opts.Cookie.HttpOnly = true;
                opts.Cookie.SecurePolicy = CookieSecurePolicy.SameAsRequest;
            });

            services.AddWebApiServices<ApiGatewayRestClientSetup>();

            services.AddCachedMethodService(new AareonUK360.Common.Mvc.Services.CachedMethodServiceOptions
            {
                ShortCachePeriod = TimeSpan.FromMinutes(5),
                LongCachePeriod = TimeSpan.FromMinutes(30)
            });

            services.AddFirstTouchFormFiles();

            services.AddWebsiteVersionHelperService();

            services.Configure<MvcNewtonsoftJsonOptions>(options =>
            {
                options.SerializerSettings.ContractResolver = new Newtonsoft.Json.Serialization.DefaultContractResolver();
                options.SerializerSettings.Converters.Add(new Newtonsoft.Json.Converters.StringEnumConverter());
            });

            services.AddScoped<IWidgetContext, WidgetContext>();
            services.AddScoped<ITenancyResolver, TenantResolver>();
            services.AddScoped<ICollectionsCache, CollectionCacheService>();
            services.AddScoped<IImageCaching, ImageCaching>();
            services.AddScoped<IWidgetAuthenticationHandler, WidgetAuthenticationHandler>();
            services.AddSingleton<EncryptionHelperService>();
            services.AddSingleton<SecureTusDiskStore>();
            services.AddSingleton(TusConfiguration.CreateTusConfiguration);
            services.AddScoped<SecureUrlService>();
            services.AddScoped<FileLinkGeneratorService>();

            // Background Services
            services.AddHostedService<TusExpiredFileDeletionBackgroundService>();

            services.AddHealthChecks()
                .AddIdentityServer(new Uri(Configuration.Security.SecurityTokenServiceUrl), name: "security token service")
                .AddUrlGroup(new Uri(new Uri(Configuration.ApiGatewayUrl), "/service-health"), name: "api gateway service");
        }

[marlon-tucker]

A bit closer,

I added a test HttpClient to one of our project modules to see if the NRE is thrown for any typed HttpClient, and it does. I then used a bit of refection to mimic what the net core code is doing:

            var registryType = AppDomain.CurrentDomain.GetAssemblies().SelectMany(a => a.GetTypes())
                .SingleOrDefault(t => t.Name == "HttpClientMappingRegistry");

            if (registryType != null)
            {
                var desc = services.Single(sd =>
                {
                    return sd.ServiceType == registryType;
                });

            }




            services.AddHttpClient<HttpClientTest>();

Turns out the service descriptor for HttpClientMappingRegistry has somehow been swapped out for one which uses a implementation factory, which the dot net core code is not expecting:

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

I'm now trying to debug how that service descriptor is being added, considering the type is an internal Microsoft class.

[marlon-tucker]

I found out why it's happening on our code base. In OrchardCore.Environment.Shell.Builders.ServiceProviderExtensions.CreateChildContainer(...) it creates a cloned IServiceCollection and the HttpClientMappingRegistry gets cloned as a factory method because it does not implement IDisposable.

The code for AddHttpClient assumes this service has an instance. IMO slightly hacky of them but it is what it is. This pull request covers the change.

https://github.com/dotnet/extensions/pull/1563

It works if I use the OrchardCore.Cms.Web project because it only adds a typed http client in an orchard module, so the code adds HttpClientMappingRegistry into the FeatureAwareServiceCollection as an instance singleton. Whereas we add various HttpClients in the main web applications Startup as well.

[deanmarcussen]

@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.

[marlon-tucker]

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

Will doing that add them into the FeatureAwareServiceCollection?

[marlon-tucker]

I have edited the issue to reflect the current findings. However I'm facing an issue, in my web app StartUp I have this in the Configure method:

            app.UseEndpoints(endpoints =>
            {
                endpoints.MapHealthChecks("/service-health", new Microsoft.AspNetCore.Diagnostics.HealthChecks.HealthCheckOptions
                {
                    ResponseWriter = HealthChecks.UI.Client.UIResponseWriter.WriteHealthCheckUIResponse
                });

                endpoints.MapGet(TusConfiguration.TUS_PROTOCOL_PATH_GET, DownloadFileEndpoint.HandleRoute);
            });

This expects to find a HealthCheckService in the service provider, which is not present as I changed those registrations to happen in the AddOrchardCms().ConfigureServices() due to services.AddHealthChecks() adding a HttpClient. Is there a work around I can do to register the health check endpoints after the shell initiation?

[marlon-tucker]

I have attempted to use RegisterStartup on the OrchardCoreBuilder to register the routes, however it seems both ConfigureServices and Configure on the Startup classes that are registered with RegisterStartup are executed twice during the shell startup process, resulting in duplicated routes being added

[deanmarcussen]

try moving your app.UseEndPoints code to AddOrchardCms().Configure((app, etc, etc)) =>

[marlon-tucker]

I couldn't find that extension method, but AddOrchardCms() has an overload which takes an Action<IApplicationBuilder>. However this appears to register the routes before the shell services have been initiated as the MapHealthChecks fails to find the requires service.

I managed to get it working, it seems the RegisterStartup call is not required, as Orchard finds the Module Startup file with reflection.

I think both cases need to be catered for, as if the startup class is explicitly registered, Orchard should not add a duplicate entry.

[jtkech]

@marlon-tucker

As @deanmarcussen said you can use AddOrchardCms().ConfigureServices() and also AddOrchardCms().Configure(). If you were able to use the 1st one you should be able to use the 2nd one. These helpers is part of the things that allow the application to behave as a module and then collaborate at the tenant level, without having to write a module.

With this 2 helpers it's like you are writing a module startup, they are different overloads but all of them, after the delegate, have a last optional Order param (0 by default), module startups also have an overridable Order property, so that, when we compose a given tenant, you can control the order of registrations / configurations of all its enabled modules including what you did with this 2 helpers from the application.

At the tenant level we discover automatically module startups without having to register them and exclude the one defined at the app level which is executed as for any app. Yes we use reflexion to discover module statups and then we register them in the tenant container, from the application with the 2 above helpers we also register custom startup classes so that their related registrations / configurations are executed when we compose a given tenant

[marlon-tucker]

Apologies, I now realise I was looking at UseOrchardCms().Configure(...) instead of AddOrchardCms().Configure(...).

Do you mind if I ask what the use case for the AddOrchardCms().RegisterStartup(...) method is? As if you are able to reference the module startup class to call that method, then it will also be in the dependency list of the application.

If I called RegisterStartup, the startup methods would be executed twice, due to the class being added to the service collection twice.

[jtkech]

From the app, RegisterStartup() may replace the usage of the 2 above helpers by defining at the app level a startup class (but with another name than the main startup) and that inherits from StartupBase that implements IStartup.

We use it here to register the startup class of OC.Mvc.Core that is a core project (not a module)

In fact, when we compose a tenant we discover all module startups (of any name) that are StartupBase including the ones defined at the app level, and we discover all module startups (whose name is Startup) that are not StartupBase but not including the ones defined at the app level.

[marlon-tucker]

From the app, RegisterStartup() may replace the usage of the 2 above helpers by defining at the app level a startup class (but with another name than the main startup) and that inherits from StartupBase that implements IStartup.

That's what I did, I created a class called OrchardStartup which inherited from StartupBase and registered it with RegisterStartup().

However, when the application ran, both the Configure and ConfigureServices methods inside the OrchardStartup class were executed twice (which lead to duplicate endpoints being created).

In fact, when we compose a tenant we discover all module startups (of any name) that are StartupBase including the ones defined at the app level, and we discover all module startups (whose name is Startup) that are not StartupBase but not including the ones defined at the app level.

I would guess this process was also adding my OrchardStartup class?

[jtkech]

Okay i will try it soon ;)

[jtkech]

@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

[marlon-tucker]

@jtkech I'm not sure this issue should be closed from the PR you did as the PR only fixes a problem I encountered when attempting to work around the original issue I submitted.

[jtkech]

Okay, i thought that the first part was solved with the provided infos, and that the only remaining part was only related to the duplicate statup when using RegisterStartup() from the application.

Feel free to open a new issue but taking into account all the infos already provided here, as the usage of the 2 OrchardCoreBuilder helpers, and knowing that RegisterStartup() doesn't register a duplicate anymore.

Sorry, you will have to explain again what is your remaining issue, but i think it's better to open a new issue if here we already have mixed things.

[marlon-tucker]

Fair enough, It's just quite the gotcha if anyone does add a HttpClient service in the main application's ConfigureServices method, as the exception that is produced is not at all helpful in determining the underlying cause.

Though at least now if they search, they should find this issue and see the work around.