Skip to content

Commit 0f06725

Browse files
authored
Update places where BindConfiguration is called, which registers anonymous delegates, and is thus not safe to be called multiple times. (#1609)
With a pre-check in place, several Try* methods behind it can be replaced for efficiency, but only if: - The implementation is an internal type and does not implement a public interface - The implementation type is not shared between Steeltoe components
1 parent cfc15ca commit 0f06725

File tree

8 files changed

+94
-21
lines changed

8 files changed

+94
-21
lines changed

src/Configuration/src/CloudFoundry/CloudFoundryServiceCollectionExtensions.cs

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,25 @@ public static IServiceCollection AddCloudFoundryOptions(this IServiceCollection
2828
{
2929
ArgumentNullException.ThrowIfNull(services);
3030

31-
services.AddOptions<CloudFoundryServicesOptions>().BindConfiguration("vcap");
31+
if (!IsRegistered(services))
32+
{
33+
services.AddOptions<CloudFoundryServicesOptions>().BindConfiguration("vcap");
3234

33-
services.TryAddEnumerable(ServiceDescriptor.Singleton<IConfigureOptions<ApplicationInstanceInfo>, ConfigureApplicationInstanceInfo>());
34-
services.TryAddEnumerable(ServiceDescriptor.Singleton<IConfigureOptions<CloudFoundryApplicationOptions>, ConfigureCloudFoundryApplicationOptions>());
35+
services.TryAddEnumerable(ServiceDescriptor.Singleton<IConfigureOptions<ApplicationInstanceInfo>, ConfigureApplicationInstanceInfo>());
36+
services.AddSingleton<IConfigureOptions<CloudFoundryApplicationOptions>, ConfigureCloudFoundryApplicationOptions>();
3537

36-
services.Replace(ServiceDescriptor.Singleton<IApplicationInstanceInfo>(serviceProvider =>
37-
{
38-
var optionsMonitor = serviceProvider.GetRequiredService<IOptionsMonitor<CloudFoundryApplicationOptions>>();
39-
return optionsMonitor.CurrentValue;
40-
}));
38+
services.Replace(ServiceDescriptor.Singleton<IApplicationInstanceInfo>(serviceProvider =>
39+
{
40+
var optionsMonitor = serviceProvider.GetRequiredService<IOptionsMonitor<CloudFoundryApplicationOptions>>();
41+
return optionsMonitor.CurrentValue;
42+
}));
43+
}
4144

4245
return services;
4346
}
47+
48+
private static bool IsRegistered(IServiceCollection services)
49+
{
50+
return services.Any(descriptor => descriptor.SafeGetImplementationType() == typeof(ConfigureCloudFoundryApplicationOptions));
51+
}
4452
}

src/Configuration/test/CloudFoundry.Test/CloudFoundryHostBuilderExtensionsTest.cs

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,4 +63,18 @@ public void HostApplicationAddCloudFoundryConfiguration_Adds()
6363

6464
configurationRoot.EnumerateProviders<CloudFoundryConfigurationProvider>().Should().ContainSingle();
6565
}
66+
67+
[Fact]
68+
public void Does_not_register_multiple_times()
69+
{
70+
WebApplicationBuilder hostBuilder = TestWebApplicationBuilderFactory.Create();
71+
hostBuilder.AddCloudFoundryConfiguration();
72+
int beforeSourceCount = hostBuilder.Configuration.EnumerateSources().Count();
73+
int beforeServiceCount = hostBuilder.Services.Count;
74+
75+
hostBuilder.AddCloudFoundryConfiguration();
76+
77+
hostBuilder.Configuration.EnumerateSources().Count().Should().Be(beforeSourceCount);
78+
hostBuilder.Services.Count.Should().Be(beforeServiceCount);
79+
}
6680
}

src/Discovery/src/Configuration/ConfigurationServiceCollectionExtensions.cs

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44

55
using Microsoft.Extensions.DependencyInjection;
66
using Microsoft.Extensions.DependencyInjection.Extensions;
7+
using Steeltoe.Common;
78
using Steeltoe.Common.Discovery;
89

910
namespace Steeltoe.Discovery.Configuration;
@@ -49,11 +50,19 @@ public static IServiceCollection AddConfigurationDiscoveryClient(this IServiceCo
4950
{
5051
ArgumentNullException.ThrowIfNull(services);
5152

52-
services.AddOptions<ConfigurationDiscoveryOptions>().BindConfiguration(ConfigurationDiscoveryOptions.ConfigurationPrefix);
53+
if (!IsRegistered(services))
54+
{
55+
services.AddOptions<ConfigurationDiscoveryOptions>().BindConfiguration(ConfigurationDiscoveryOptions.ConfigurationPrefix);
5356

54-
services.TryAddEnumerable(ServiceDescriptor.Singleton<IDiscoveryClient, ConfigurationDiscoveryClient>());
55-
services.AddHostedService<DiscoveryClientHostedService>();
57+
services.TryAddEnumerable(ServiceDescriptor.Singleton<IDiscoveryClient, ConfigurationDiscoveryClient>());
58+
services.AddHostedService<DiscoveryClientHostedService>();
59+
}
5660

5761
return services;
5862
}
63+
64+
private static bool IsRegistered(IServiceCollection services)
65+
{
66+
return services.Any(descriptor => descriptor.SafeGetImplementationType() == typeof(ConfigurationDiscoveryClient));
67+
}
5968
}

src/Discovery/src/Consul/ConsulServiceCollectionExtensions.cs

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
using Microsoft.Extensions.DependencyInjection;
77
using Microsoft.Extensions.DependencyInjection.Extensions;
88
using Microsoft.Extensions.Options;
9+
using Steeltoe.Common;
910
using Steeltoe.Common.Discovery;
1011
using Steeltoe.Common.Extensions;
1112
using Steeltoe.Common.HealthChecks;
@@ -32,12 +33,20 @@ public static IServiceCollection AddConsulDiscoveryClient(this IServiceCollectio
3233
{
3334
ArgumentNullException.ThrowIfNull(services);
3435

35-
ConfigureConsulServices(services);
36-
AddConsulServices(services);
36+
if (!IsRegistered(services))
37+
{
38+
ConfigureConsulServices(services);
39+
AddConsulServices(services);
40+
}
3741

3842
return services;
3943
}
4044

45+
private static bool IsRegistered(IServiceCollection services)
46+
{
47+
return services.Any(descriptor => descriptor.SafeGetImplementationType() == typeof(PostConfigureConsulDiscoveryOptions));
48+
}
49+
4150
private static void ConfigureConsulServices(IServiceCollection services)
4251
{
4352
services.AddApplicationInstanceInfo();
@@ -96,6 +105,6 @@ private static void AddConsulServices(IServiceCollection services)
96105
services.AddSingleton<ConsulServiceRegistrar>();
97106
services.TryAddEnumerable(ServiceDescriptor.Singleton<IDiscoveryClient, ConsulDiscoveryClient>());
98107
services.AddHostedService<DiscoveryClientHostedService>();
99-
services.TryAddEnumerable(ServiceDescriptor.Singleton<IHealthContributor, ConsulHealthContributor>());
108+
services.AddSingleton<IHealthContributor, ConsulHealthContributor>();
100109
}
101110
}

src/Discovery/src/Eureka/EurekaServiceCollectionExtensions.cs

Lines changed: 11 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ public static IServiceCollection AddEurekaDiscoveryClient(this IServiceCollectio
3535
{
3636
ArgumentNullException.ThrowIfNull(services);
3737

38-
if (services.All(descriptor => descriptor.SafeGetImplementationType() != typeof(EurekaDiscoveryClient)))
38+
if (!IsRegistered(services))
3939
{
4040
ConfigureEurekaServices(services);
4141
AddEurekaServices(services);
@@ -44,6 +44,11 @@ public static IServiceCollection AddEurekaDiscoveryClient(this IServiceCollectio
4444
return services;
4545
}
4646

47+
private static bool IsRegistered(IServiceCollection services)
48+
{
49+
return services.Any(descriptor => descriptor.SafeGetImplementationType() == typeof(EurekaDiscoveryClient));
50+
}
51+
4752
private static void ConfigureEurekaServices(IServiceCollection services)
4853
{
4954
services.AddApplicationInstanceInfo();
@@ -86,9 +91,9 @@ private static void ConfigureEurekaInstanceOptions(IServiceCollection services)
8691
private static void AddEurekaServices(IServiceCollection services)
8792
{
8893
services.TryAddSingleton(TimeProvider.System);
89-
services.AddSingleton<HealthCheckHandlerProvider>();
90-
services.TryAddEnumerable(ServiceDescriptor.Singleton<IHealthContributor, EurekaServerHealthContributor>());
91-
services.AddSingleton<EurekaApplicationInfoManager>();
94+
services.TryAddSingleton<HealthCheckHandlerProvider>();
95+
services.AddSingleton<IHealthContributor, EurekaServerHealthContributor>();
96+
services.TryAddSingleton<EurekaApplicationInfoManager>();
9297

9398
services.TryAddSingleton<EurekaDiscoveryClient>();
9499
services.AddSingleton<IDiscoveryClient>(serviceProvider => serviceProvider.GetRequiredService<EurekaDiscoveryClient>());
@@ -103,9 +108,9 @@ private static void AddEurekaServices(IServiceCollection services)
103108
private static void AddEurekaClient(IServiceCollection services)
104109
{
105110
services.TryAddSingleton<HttpClientHandlerFactory>();
106-
services.TryAddSingleton<ValidateCertificatesHttpClientHandlerConfigurer<EurekaClientOptions>>();
111+
services.AddSingleton<ValidateCertificatesHttpClientHandlerConfigurer<EurekaClientOptions>>();
107112
services.TryAddSingleton<ClientCertificateHttpClientHandlerConfigurer>();
108-
services.TryAddSingleton<EurekaHttpClientHandlerConfigurer>();
113+
services.AddSingleton<EurekaHttpClientHandlerConfigurer>();
109114
services.ConfigureCertificateOptions("Eureka");
110115

111116
IHttpClientBuilder eurekaHttpClientBuilder = services.AddHttpClient("Eureka");

src/Discovery/test/Configuration.Test/ConfigurationDiscoveryClientTest.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,4 +179,16 @@ public async Task RegistersHostedService()
179179

180180
serviceProvider.GetServices<IHostedService>().OfType<DiscoveryClientHostedService>().Should().ContainSingle();
181181
}
182+
183+
[Fact]
184+
public void Does_not_register_multiple_times()
185+
{
186+
var services = new ServiceCollection();
187+
services.AddConfigurationDiscoveryClient();
188+
int beforeServiceCount = services.Count;
189+
190+
services.AddConfigurationDiscoveryClient();
191+
192+
services.Count.Should().Be(beforeServiceCount);
193+
}
182194
}

src/Discovery/test/HttpClients.Test/RegisterMultipleDiscoveryClientsTest.cs

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -873,4 +873,20 @@ public async Task WithMultipleClients_AddsDiscoveryClients()
873873
serviceProvider.GetServices<IHealthContributor>().OfType<EurekaServerHealthContributor>().Should().ContainSingle();
874874
serviceProvider.GetServices<IHealthContributor>().OfType<EurekaApplicationsHealthContributor>().Should().BeEmpty();
875875
}
876+
877+
[Fact]
878+
public void WithMultipleClients_DotNotRegisterMultipleTimes()
879+
{
880+
var services = new ServiceCollection();
881+
services.AddConfigurationDiscoveryClient();
882+
services.AddConsulDiscoveryClient();
883+
services.AddEurekaDiscoveryClient();
884+
int beforeServiceCount = services.Count;
885+
886+
services.AddConfigurationDiscoveryClient();
887+
services.AddConsulDiscoveryClient();
888+
services.AddEurekaDiscoveryClient();
889+
890+
services.Count.Should().Be(beforeServiceCount);
891+
}
876892
}

src/Logging/src/DynamicSerilog/SerilogLoggingBuilderExtensions.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,7 @@ public static ILoggingBuilder AddDynamicSerilog(this ILoggingBuilder builder, Lo
104104
private static bool IsSerilogDynamicLoggerProviderAlreadyRegistered(ILoggingBuilder builder)
105105
{
106106
return builder.Services.Any(descriptor =>
107-
descriptor.ServiceType == typeof(ILoggerProvider) && descriptor.SafeGetImplementationType() == typeof(DynamicSerilogLoggerProvider));
107+
descriptor.SafeGetImplementationType() == typeof(DynamicSerilogLoggerProvider) && descriptor.ServiceType == typeof(ILoggerProvider));
108108
}
109109

110110
private static void AssertNoDynamicLoggerProviderRegistered(ILoggingBuilder builder)

0 commit comments

Comments
 (0)