Skip to content

Commit 722638c

Browse files
twsouthwicktwsouthwick
authored
Use Try... pattern to register services to minimize duplication (#516)
If someone calls `.AddSystemWebAdapters()` multiple times in order to add more services to it, it registers the initial services multiple times as well. This is ok, except that the startup filter we use to register middleware will now exist multiple times and will register duplicated features on each invocation
1 parent ef9aaae commit 722638c

File tree

5 files changed

+136
-49
lines changed

5 files changed

+136
-49
lines changed

‎src/Microsoft.AspNetCore.SystemWebAdapters.CoreServices/HostingRuntimeExtensions.cs‎

Lines changed: 48 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212
using Microsoft.Extensions.Configuration;
1313
using Microsoft.Extensions.DependencyInjection;
1414
using Microsoft.Extensions.DependencyInjection.Extensions;
15+
using Microsoft.Extensions.Options;
1516

1617
namespace Microsoft.AspNetCore.SystemWebAdapters;
1718

@@ -24,41 +25,59 @@ public static void AddHostingRuntime(this IServiceCollection services)
2425
services.TryAddSingleton<IMapPathUtility, MapPathUtility>();
2526
services.TryAddEnumerable(ServiceDescriptor.Transient<IStartupFilter, HostingEnvironmentStartupFilter>());
2627

27-
services.AddOptions<SystemWebAdaptersOptions>()
28+
services.TryAddEnumerable(ServiceDescriptor.Transient<IConfigureOptions<SystemWebAdaptersOptions>, OlderIISModuleSupport>());
29+
services.TryAddEnumerable(ServiceDescriptor.Transient<IConfigureOptions<SystemWebAdaptersOptions>, EnvironmentFeatureConfigureOptions>());
30+
services.TryAddEnumerable(ServiceDescriptor.Transient<IConfigureOptions<SystemWebAdaptersOptions>, DefaultAppPathConfigureOptions>());
31+
}
2832

29-
// This configures for anyone using older IIS modules that don't set the values (and to maintain behavior with the adapters <1.3)
30-
.Configure(options =>
33+
/// <summary>
34+
/// On .NET 8+, IIISEnvironmentFeature is available by default if running on IIS. We have an internal version
35+
/// we load at startup so that regardless of version and server this may be available (for example, in case some
36+
/// one wants to set the environment variables on a Kestrel hosted system to get the behavior)
37+
/// </summary>
38+
private sealed class EnvironmentFeatureConfigureOptions(IServer server) : IConfigureOptions<SystemWebAdaptersOptions>
39+
{
40+
public void Configure(SystemWebAdaptersOptions options)
41+
{
42+
if (server.Features.Get<IIISEnvironmentFeature>() is { } feature)
3143
{
32-
options.IsHosted = true;
44+
options.ApplicationPhysicalPath = feature.ApplicationPhysicalPath;
45+
options.ApplicationVirtualPath = feature.ApplicationVirtualPath;
46+
options.ApplicationID = feature.ApplicationId;
47+
options.SiteName = feature.SiteName;
48+
}
49+
}
50+
}
3351

34-
if (NativeMethods.IsAspNetCoreModuleLoaded())
35-
{
36-
var config = NativeMethods.HttpGetApplicationProperties();
52+
/// <summary>
53+
/// On ASP.NET Core this should be the same. We're doing it here rather than a PostConfigure because someone may want to set it up differently
54+
/// </summary>
55+
private sealed class DefaultAppPathConfigureOptions : IConfigureOptions<SystemWebAdaptersOptions>
56+
{
57+
public void Configure(SystemWebAdaptersOptions options)
58+
{
59+
options.AppDomainAppPath = options.ApplicationPhysicalPath;
60+
options.AppDomainAppVirtualPath = options.ApplicationVirtualPath;
61+
}
62+
}
3763

38-
options.ApplicationPhysicalPath = config.pwzFullApplicationPath;
39-
options.ApplicationVirtualPath = config.pwzVirtualApplicationPath;
40-
}
41-
})
64+
/// <summary>
65+
/// This configures for anyone using older IIS modules that don't set the values (and to maintain behavior with the adapters <1.3)
66+
/// </summary>
67+
private sealed class OlderIISModuleSupport : IConfigureOptions<SystemWebAdaptersOptions>
68+
{
69+
public void Configure(SystemWebAdaptersOptions options)
70+
{
71+
options.IsHosted = true;
4272

43-
// On .NET 8+, IIISEnvironmentFeature is available by default if running on IIS. We have an internal version
44-
// we load at startup so that regardless of version and server this may be available (for example, in case some
45-
// one wants to set the environment variables on a Kestrel hosted system to get the behavior)
46-
.Configure<IServer>((options, server) =>
73+
if (NativeMethods.IsAspNetCoreModuleLoaded())
4774
{
48-
if (server.Features.Get<IIISEnvironmentFeature>() is { } feature)
49-
{
50-
options.ApplicationPhysicalPath = feature.ApplicationPhysicalPath;
51-
options.ApplicationVirtualPath = feature.ApplicationVirtualPath;
52-
options.ApplicationID = feature.ApplicationId;
53-
options.SiteName = feature.SiteName;
54-
}
55-
})
56-
.Configure(options =>
57-
{
58-
// On ASP.NET Core this should be the same. We're doing it here rather than a PostConfigure because someone may want to set it up differently
59-
options.AppDomainAppPath = options.ApplicationPhysicalPath;
60-
options.AppDomainAppVirtualPath = options.ApplicationVirtualPath;
61-
});
75+
var config = NativeMethods.HttpGetApplicationProperties();
76+
77+
options.ApplicationPhysicalPath = config.pwzFullApplicationPath;
78+
options.ApplicationVirtualPath = config.pwzVirtualApplicationPath;
79+
}
80+
}
6281
}
6382

6483
private sealed class HostingEnvironmentStartupFilter : IStartupFilter

‎src/Microsoft.AspNetCore.SystemWebAdapters.CoreServices/HttpApplication/HttpApplicationExtensions.cs‎

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -18,14 +18,23 @@ namespace Microsoft.Extensions.DependencyInjection;
1818
public static class HttpApplicationExtensions
1919
{
2020
public static ISystemWebAdapterBuilder AddHttpApplication(this ISystemWebAdapterBuilder builder)
21-
=> builder.AddHttpApplication(_ => { });
21+
{
22+
ArgumentNullException.ThrowIfNull(builder);
23+
24+
return builder.AddHttpApplicationInternal(null);
25+
}
2226

2327
public static ISystemWebAdapterBuilder AddHttpApplication(this ISystemWebAdapterBuilder builder, Action<HttpApplicationOptions> configure)
2428
{
2529
ArgumentNullException.ThrowIfNull(builder);
2630

31+
return builder.AddHttpApplicationInternal(configure);
32+
}
33+
34+
private static ISystemWebAdapterBuilder AddHttpApplicationInternal(this ISystemWebAdapterBuilder builder, Action<HttpApplicationOptions>? configure)
35+
{
2736
builder.Services.TryAddSingleton<ModuleCollection>();
28-
builder.Services.AddTransient<IModuleRegistrar>(sp => sp.GetRequiredService<ModuleCollection>());
37+
builder.Services.TryAddTransient<IModuleRegistrar>(sp => sp.GetRequiredService<ModuleCollection>());
2938
builder.Services.TryAddSingleton<HttpApplicationPooledObjectPolicy>();
3039
builder.Services.TryAddSingleton<IPooledObjectPolicy<HttpApplication>>(sp => sp.GetRequiredService<HttpApplicationPooledObjectPolicy>());
3140
builder.Services.TryAddSingleton<ObjectPool<HttpApplication>>(sp =>
@@ -41,12 +50,12 @@ public static ISystemWebAdapterBuilder AddHttpApplication(this ISystemWebAdapter
4150
return provider.Create(policy);
4251
});
4352

44-
builder.Services.AddOptions<HttpApplicationOptions>()
45-
.Configure<ModuleCollection>((options, modules) =>
46-
{
47-
options.ModuleCollection = modules;
48-
})
49-
.Configure(configure);
53+
builder.Services.TryAddEnumerable(ServiceDescriptor.Transient<IConfigureOptions<HttpApplicationOptions>, ModuleCollectionInitializeOptions>());
54+
55+
if (configure is { })
56+
{
57+
builder.Services.Configure(configure);
58+
}
5059

5160
return builder;
5261
}
@@ -149,4 +158,12 @@ public static IApplicationBuilder UseAuthorizationEvents(this IApplicationBuilde
149158

150159
internal static bool AreHttpApplicationEventsRequired(this IApplicationBuilder builder)
151160
=> builder.ApplicationServices.GetRequiredService<IOptions<HttpApplicationOptions>>().Value.IsAdded;
161+
162+
private sealed class ModuleCollectionInitializeOptions(ModuleCollection modules) : IConfigureOptions<HttpApplicationOptions>
163+
{
164+
public void Configure(HttpApplicationOptions options)
165+
{
166+
options.ModuleCollection = modules;
167+
}
168+
}
152169
}

‎src/Microsoft.AspNetCore.SystemWebAdapters.CoreServices/Mvc/MvcExtensions.cs‎

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,23 +4,28 @@
44
using Microsoft.AspNetCore.Mvc;
55
using Microsoft.AspNetCore.SystemWebAdapters;
66
using Microsoft.AspNetCore.SystemWebAdapters.Mvc;
7+
using Microsoft.Extensions.DependencyInjection.Extensions;
8+
using Microsoft.Extensions.Options;
79

810
namespace Microsoft.Extensions.DependencyInjection;
911

1012
internal static class MvcExtensions
1113
{
1214
public static ISystemWebAdapterBuilder AddMvc(this ISystemWebAdapterBuilder builder)
1315
{
14-
builder.Services.AddTransient<ResponseEndFilter>();
15-
16-
builder.Services.AddOptions<MvcOptions>()
17-
.Configure(options =>
18-
{
19-
// We want the check for HttpResponse.End() to be done as soon as possible after the action is run.
20-
// This will minimize any chance that output will be written which will fail since the response has completed.
21-
options.Filters.Add<ResponseEndFilter>(int.MaxValue);
22-
});
16+
builder.Services.TryAddTransient<ResponseEndFilter>();
17+
builder.Services.TryAddEnumerable(ServiceDescriptor.Transient<IConfigureOptions<MvcOptions>, ResponseEndFilterOptions>());
2318

2419
return builder;
2520
}
21+
22+
private sealed class ResponseEndFilterOptions : IConfigureOptions<MvcOptions>
23+
{
24+
public void Configure(MvcOptions options)
25+
{
26+
// We want the check for HttpResponse.End() to be done as soon as possible after the action is run.
27+
// This will minimize any chance that output will be written which will fail since the response has completed.
28+
options.Filters.Add<ResponseEndFilter>(int.MaxValue);
29+
}
30+
}
2631
}

‎src/Microsoft.AspNetCore.SystemWebAdapters.CoreServices/SystemWebAdaptersExtensions.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,9 +21,9 @@ public static ISystemWebAdapterBuilder AddSystemWebAdapters(this IServiceCollect
2121
{
2222
services.AddHttpContextAccessor();
2323
services.TryAddSingleton(TimeProvider.System);
24-
services.AddSingleton<Cache>();
25-
services.AddSingleton<IBrowserCapabilitiesFactory, BrowserCapabilitiesFactory>();
26-
services.AddTransient<IStartupFilter, HttpContextStartupFilter>();
24+
services.TryAddSingleton<Cache>();
25+
services.TryAddSingleton<IBrowserCapabilitiesFactory, BrowserCapabilitiesFactory>();
26+
services.TryAddEnumerable(ServiceDescriptor.Transient<IStartupFilter, HttpContextStartupFilter>());
2727
services.AddHostingRuntime();
2828

2929
return new SystemWebAdapterBuilder(services)

‎test/Microsoft.AspNetCore.SystemWebAdapters.CoreServices.Tests/BuilderTests.cs‎

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using Microsoft.Extensions.DependencyInjection;
5+
using Xunit;
6+
7+
namespace Microsoft.AspNetCore.SystemWebAdapters;
8+
9+
public class BuilderTests
10+
{
11+
[Fact]
12+
public void MultipleServiceRegistrationInvocations()
13+
{
14+
// Arrange
15+
var services = new ServiceCollection();
16+
17+
services.AddSystemWebAdapters();
18+
19+
var count = services.Count;
20+
21+
// Act
22+
services.AddSystemWebAdapters();
23+
24+
// Assert
25+
Assert.Equal(count, services.Count);
26+
}
27+
28+
[Fact]
29+
public void MultipleServiceHttpApplicationRegistrationInvocations()
30+
{
31+
// Arrange
32+
var services = new ServiceCollection();
33+
34+
services.AddSystemWebAdapters()
35+
.AddHttpApplication();
36+
37+
var count = services.Count;
38+
39+
// Act
40+
services.AddSystemWebAdapters()
41+
.AddHttpApplication();
42+
43+
// Assert
44+
Assert.Equal(count, services.Count);
45+
}
46+
}

0 commit comments

Comments
 (0)