Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add the ability to start and stop IHostedService instances concurrently #75894

Closed
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ public partial class HostOptions
{
public HostOptions() { }
public Microsoft.Extensions.Hosting.BackgroundServiceExceptionBehavior BackgroundServiceExceptionBehavior { get { throw null; } set { } }
public bool ServicesStartConcurrently { get { throw null; } set { } }
public bool ServicesStopConcurrently { get { throw null; } set { } }
public System.TimeSpan ShutdownTimeout { get { throw null; } set { } }
}
}
Expand Down
26 changes: 25 additions & 1 deletion src/libraries/Microsoft.Extensions.Hosting/src/HostOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,16 @@ public class HostOptions
/// </summary>
public TimeSpan ShutdownTimeout { get; set; } = TimeSpan.FromSeconds(30);

/// <summary>
/// Determines if the <see cref="IHost"/> will start registered instances of <see cref="IHostedService"/> concurrently or sequentially
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to document the default.

/// </summary>
public bool ServicesStartConcurrently { get; set; }

/// <summary>
/// Determines if the <see cref="IHost"/> will stop registered instances of <see cref="IHostedService"/> concurrently or sequentially
/// </summary>
public bool ServicesStopConcurrently { get; set; }

/// <summary>
/// The behavior the <see cref="IHost"/> will follow when any of
/// its <see cref="BackgroundService"/> instances throw an unhandled exception.
Expand All @@ -30,11 +40,25 @@ public class HostOptions
internal void Initialize(IConfiguration configuration)
{
var timeoutSeconds = configuration["shutdownTimeoutSeconds"];
if (!string.IsNullOrEmpty(timeoutSeconds)
if (!string.IsNullOrWhiteSpace(timeoutSeconds)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems unrelated/unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you would want to use NullOrEmpty over NullOrWhiteSpace here. The TryParse methods further down the chain will handle the blank-space scenarios, but I don't see a need to even get there when we can stop it so easily.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The odds that the config value is going to be just whitespace are VERY low. So doing extra checking just slows down the happy path.

Look at the implementation of IsNullOrWhiteSpace:

public static bool IsNullOrWhiteSpace([NotNullWhen(false)] string? value)
{
if (value == null) return true;
for (int i = 0; i < value.Length; i++)
{
if (!char.IsWhiteSpace(value[i])) return false;
}
return true;
}

It goes through the string checking the chars one by one. There is no reason to do this.

&& int.TryParse(timeoutSeconds, NumberStyles.None, CultureInfo.InvariantCulture, out var seconds))
{
ShutdownTimeout = TimeSpan.FromSeconds(seconds);
}

var servicesStartConcurrently = configuration["servicesStartConcurrently"];
if (!string.IsNullOrWhiteSpace(servicesStartConcurrently)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!string.IsNullOrWhiteSpace(servicesStartConcurrently)
if (!string.IsNullOrEmpty(servicesStartConcurrently)

&& bool.TryParse(servicesStartConcurrently, out bool startBehavior))
{
ServicesStartConcurrently = startBehavior;
}

var servicesStopConcurrently = configuration["servicesStopConcurrently"];
if (!string.IsNullOrWhiteSpace(servicesStopConcurrently)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!string.IsNullOrWhiteSpace(servicesStopConcurrently)
if (!string.IsNullOrEmpty(servicesStopConcurrently)

&& bool.TryParse(servicesStopConcurrently, out bool stopBehavior))
{
ServicesStopConcurrently = stopBehavior;
}
}
}
}
104 changes: 91 additions & 13 deletions src/libraries/Microsoft.Extensions.Hosting/src/Internal/Host.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.ExceptionServices;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Extensions.DependencyInjection;
Expand Down Expand Up @@ -65,14 +66,63 @@ public async Task StartAsync(CancellationToken cancellationToken = default)
combinedCancellationToken.ThrowIfCancellationRequested();
_hostedServices = Services.GetRequiredService<IEnumerable<IHostedService>>();

foreach (IHostedService hostedService in _hostedServices)
List<Exception> exceptions = new List<Exception>();

if (_options.ServicesStartConcurrently)
{
Task tasks = Task.WhenAll(_hostedServices.Select(async service =>
{
await service.StartAsync(combinedCancellationToken).ConfigureAwait(false);

if (service is BackgroundService backgroundService)
{
_ = TryExecuteBackgroundServiceAsync(backgroundService);
}
}));

try
{
await tasks.ConfigureAwait(false);
}
catch (Exception ex)
{
exceptions.AddRange(tasks.Exception?.InnerExceptions?.ToArray() ?? new[] { ex });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
exceptions.AddRange(tasks.Exception?.InnerExceptions?.ToArray() ?? new[] { ex });
exceptions.AddRange(tasks.Exception?.InnerExceptions ?? new[] { ex });

There's no need to ToArray() here. Same comment for the stopping code.

}
}
else
{
// Fire IHostedService.Start
await hostedService.StartAsync(combinedCancellationToken).ConfigureAwait(false);
foreach (IHostedService hostedService in _hostedServices)
{
try
{
// Fire IHostedService.Start
await hostedService.StartAsync(combinedCancellationToken).ConfigureAwait(false);

if (hostedService is BackgroundService backgroundService)
{
_ = TryExecuteBackgroundServiceAsync(backgroundService);
}
}
catch (Exception ex)
{
exceptions.Add(ex);
}
}
}

if (hostedService is BackgroundService backgroundService)
if (exceptions.Count > 0)
{
if (exceptions.Count == 1)
jerryk414 marked this conversation as resolved.
Show resolved Hide resolved
{
_ = TryExecuteBackgroundServiceAsync(backgroundService);
// Rethrow if it's a single error
_logger.HostedServiceStartupFaulted(exceptions[0]);
ExceptionDispatchInfo.Capture(exceptions[0]).Throw();
}
else
{
var ex = new AggregateException("One or more hosted services failed to start.", exceptions);
_logger.HostedServiceStartupFaulted(ex);
throw ex;
}
}

Expand Down Expand Up @@ -125,18 +175,37 @@ public async Task StopAsync(CancellationToken cancellationToken = default)
// Trigger IHostApplicationLifetime.ApplicationStopping
_applicationLifetime.StopApplication();

IList<Exception> exceptions = new List<Exception>();
if (_hostedServices != null) // Started?
List<Exception> exceptions = new List<Exception>();
if (_hostedServices?.Any() == true) // Started?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the change to call Any() here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose the inner block here might succeed even if the list is empty. I'm not 100% sure what happens if you pass an empty list of tasks into Task.WhenAll, so i'll have to figure that out.

I just felt like it wasn't necessary to even have to worry about that.

{
foreach (IHostedService hostedService in _hostedServices.Reverse())
// Ensure hosted services are stopped in FILO order
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Ensure hosted services are stopped in FILO order
// Ensure hosted services are stopped in LIFO order

IEnumerable<IHostedService> hostedServices = _hostedServices.Reverse();

if (_options.ServicesStopConcurrently)
{
Task tasks = Task.WhenAll(hostedServices.Select(async service => await service.StopAsync(token).ConfigureAwait(false)));

try
{
await hostedService.StopAsync(token).ConfigureAwait(false);
await tasks.ConfigureAwait(false);
}
catch (Exception ex)
{
exceptions.Add(ex);
exceptions.AddRange(tasks.Exception?.InnerExceptions?.ToArray() ?? new[] { ex });
}
}
else
{
foreach (IHostedService hostedService in hostedServices)
{
try
{
await hostedService.StopAsync(token).ConfigureAwait(false);
}
catch (Exception ex)
{
exceptions.Add(ex);
}
}
}
}
Expand All @@ -155,9 +224,18 @@ public async Task StopAsync(CancellationToken cancellationToken = default)

if (exceptions.Count > 0)
{
var ex = new AggregateException("One or more hosted services failed to stop.", exceptions);
_logger.StoppedWithException(ex);
throw ex;
if (exceptions.Count == 1)
{
// Rethrow if it's a single error
_logger.HostedServiceStartupFaulted(exceptions[0]);
ExceptionDispatchInfo.Capture(exceptions[0]).Throw();
}
else
{
var ex = new AggregateException("One or more hosted services failed to stop.", exceptions);
_logger.HostedServiceStartupFaulted(ex);
Comment on lines +231 to +237
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is stopping, not starting

Suggested change
_logger.HostedServiceStartupFaulted(exceptions[0]);
ExceptionDispatchInfo.Capture(exceptions[0]).Throw();
}
else
{
var ex = new AggregateException("One or more hosted services failed to stop.", exceptions);
_logger.HostedServiceStartupFaulted(ex);
_logger.StoppedWithException(exceptions[0]);
ExceptionDispatchInfo.Capture(exceptions[0]).Throw();
}
else
{
var ex = new AggregateException("One or more hosted services failed to stop.", exceptions);
_logger.StoppedWithException(ex);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof good catch, i don't know how I missed that

throw ex;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -101,5 +101,16 @@ public static void BackgroundServiceStoppingHost(this ILogger logger, Exception?
message: SR.BackgroundServiceExceptionStoppedHost);
}
}

public static void HostedServiceStartupFaulted(this ILogger logger, Exception? ex)
{
if (logger.IsEnabled(LogLevel.Error))
{
logger.LogError(
eventId: LoggerEventIds.HostedServiceStartupFaulted,
exception: ex,
message: "Hosting failed to start");
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,5 +17,6 @@ internal static class LoggerEventIds
public static readonly EventId ApplicationStoppedException = new EventId(8, nameof(ApplicationStoppedException));
public static readonly EventId BackgroundServiceFaulted = new EventId(9, nameof(BackgroundServiceFaulted));
public static readonly EventId BackgroundServiceStoppingHost = new EventId(10, nameof(BackgroundServiceStoppingHost));
public static readonly EventId HostedServiceStartupFaulted = new EventId(11, nameof(HostedServiceStartupFaulted));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Configuration.UserSecrets;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Moq;
using Xunit;

namespace Microsoft.Extensions.Hosting.Tests
Expand All @@ -36,6 +38,114 @@ public async Task StopAsyncWithCancellation()
await host.StopAsync(cts.Token);
}

public static IEnumerable<object[]> StartAsync_StopAsync_Concurrency_TestCases
{
get
{
foreach (bool stopConcurrently in new[] { true, false })
foreach (bool startConcurrently in new[] { true, false })
foreach (int hostedServiceCount in new[] { 0, 1, 10 })
{
yield return new object[] { stopConcurrently, startConcurrently, hostedServiceCount };
}
Comment on lines +45 to +50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
foreach (bool stopConcurrently in new[] { true, false })
foreach (bool startConcurrently in new[] { true, false })
foreach (int hostedServiceCount in new[] { 0, 1, 10 })
{
yield return new object[] { stopConcurrently, startConcurrently, hostedServiceCount };
}
foreach (bool stopConcurrently in new[] { true, false })
{
foreach (bool startConcurrently in new[] { true, false })
{
foreach (int hostedServiceCount in new[] { 0, 1, 10 })
{
yield return new object[] { stopConcurrently, startConcurrently, hostedServiceCount };
}
}
}

See rules 1 and 18 in https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/coding-style.md

}
}
[Theory]
[MemberData(nameof(StartAsync_StopAsync_Concurrency_TestCases))]
public async Task StartAsync_StopAsync_Concurrency(bool stopConcurrently, bool startConcurrently, int hostedServiceCount)
{
var startExecutionCount = 0;
var startCallOrder = 0;

var stopCallOrder = hostedServiceCount;
var stopExecutionCount = 0;

var hostedServices = new Mock<IHostedService>[hostedServiceCount];

for (int i = 0; i < hostedServiceCount; i++)
{
var index = i;
var service = new Mock<IHostedService>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using Moq, would it make it easier to understand to reuse the DelegateHostedService class here?

service.Setup(y => y.StopAsync(It.IsAny<CancellationToken>()))
.Callback(() =>
{
// Ensures that IHostedService instances are stopped in FILO order
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Ensures that IHostedService instances are stopped in FILO order
// Ensures that IHostedService instances are stopped in LIFO order

LIFO is the more common term

Assert.Equal(index, --stopCallOrder);
})
.Returns(async () =>
{
if (stopConcurrently && hostedServiceCount > 1)
{
// Basically this will force all IHostedService instances to have StopAsync called before any one completes
stopExecutionCount++;
int waitCount = 0;

while (stopExecutionCount < hostedServiceCount && waitCount++ < 20)
{
await Task.Delay(10).ConfigureAwait(false);
}

// This will ensure that we truly are stopping all services asynchronously
Assert.Equal(hostedServiceCount, stopExecutionCount);
Comment on lines +83 to +89
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a recipe for a flaky test. There can't be any race conditions in our tests, as they are executed many, many times in every PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah.. i knew it wasn't the best, it's been a while since i actually wrote this so i can't remember what i was thinking exactly, but i think i can find a better way.

}
})
.Verifiable();

service.Setup(y => y.StartAsync(It.IsAny<CancellationToken>()))
.Callback(() =>
{
// Ensures that IHostedService instances are started in FILO order
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Ensures that IHostedService instances are started in FILO order
// Ensures that IHostedService instances are started in FIFO order

Assert.Equal(index, startCallOrder++);
})
.Returns(async () =>
{
if (startConcurrently && hostedServiceCount > 1)
{
// Basically this will force all IHostedService instances to have StartAsync called before any one completes
startExecutionCount++;
int waitCount = 0;

while (startExecutionCount < hostedServiceCount && waitCount++ < 20)
{
await Task.Delay(10).ConfigureAwait(false);
}

// This will ensure that we truly are starting all services asynchronously
Assert.Equal(hostedServiceCount, startExecutionCount);
}
})
.Verifiable();

hostedServices[index] = service;
}

var builder = new HostBuilder();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var builder = new HostBuilder();

Doesn't appear to be used.

using var host = Host.CreateDefaultBuilder().ConfigureHostConfiguration(configBuilder =>
{
configBuilder.AddInMemoryCollection(new KeyValuePair<string, string>[]
{
new KeyValuePair<string, string>("servicesStartConcurrently", startConcurrently.ToString()),
new KeyValuePair<string, string>("servicesStopConcurrently", stopConcurrently.ToString())
});
}).ConfigureServices(configurer =>
{
for (int i = 0; i < hostedServiceCount; i++)
{
var index = i;
configurer.Add(ServiceDescriptor.Singleton<IHostedService>(hostedServices[index].Object));
}
Comment on lines +132 to +136
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (int i = 0; i < hostedServiceCount; i++)
{
var index = i;
configurer.Add(ServiceDescriptor.Singleton<IHostedService>(hostedServices[index].Object));
}
foreach (Mock<IHostedService> hostedService in hostedServices)
{
configurer.Add(ServiceDescriptor.Singleton<IHostedService>(hostedService.Object));
}

}).Build();

await host.StartAsync(CancellationToken.None);

await host.StopAsync(CancellationToken.None);

foreach (var service in hostedServices)
{
service.Verify();
}
}

[Fact]
public void CreateDefaultBuilder_IncludesContentRootByDefault()
{
Expand Down