Skip to content
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add missing refs; add unit tests; ensure stop behavior cannot be chan…
…ged mid-execution; add support for stop behavior and exception behavior to be set in config file
  • Loading branch information
jerryk414 committed May 6, 2022
commit 3798699c39dfeb391fc481d5431fc35df4278f23
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ public enum BackgroundServiceExceptionBehavior
StopHost,
Ignore
}
public enum BackgroundServiceStopBehavior
{
Sequential,
Asynchronous
}
public partial class ConsoleLifetimeOptions
{
public ConsoleLifetimeOptions() { }
Expand Down Expand Up @@ -104,6 +109,7 @@ public partial class HostOptions
public HostOptions() { }
public System.TimeSpan ShutdownTimeout { get { throw null; } set { } }
public Microsoft.Extensions.Hosting.BackgroundServiceExceptionBehavior BackgroundServiceExceptionBehavior { get { throw null; } set { } }
public Microsoft.Extensions.Hosting.BackgroundServiceStopBehavior BackgroundServiceStopBehavior { get { throw null; } set { } }
}
}
namespace Microsoft.Extensions.Hosting.Internal
Expand Down
16 changes: 15 additions & 1 deletion src/libraries/Microsoft.Extensions.Hosting/src/HostOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,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 backgroundServiceStopBehavior = configuration["backgroundServiceStopBehavior"];
if (!string.IsNullOrWhiteSpace(backgroundServiceStopBehavior)
&& Enum.TryParse<BackgroundServiceStopBehavior>(backgroundServiceStopBehavior, out var stopBehavior))
{
BackgroundServiceStopBehavior = stopBehavior;
}

var backgroundServiceExceptionBehavior = configuration["backgroundServiceExceptionBehavior"];
if (!string.IsNullOrWhiteSpace(backgroundServiceExceptionBehavior)
&& Enum.TryParse<BackgroundServiceExceptionBehavior>(backgroundServiceExceptionBehavior, out var exceptionBehavior))
{
BackgroundServiceExceptionBehavior = exceptionBehavior;
}
}
}
}
49 changes: 24 additions & 25 deletions src/libraries/Microsoft.Extensions.Hosting/src/Internal/Host.cs
Original file line number Diff line number Diff line change
Expand Up @@ -120,15 +120,17 @@ 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.

{
Queue<Task> tasks = new Queue<Task>();
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();

switch (_options.BackgroundServiceStopBehavior)
{
switch (_options.BackgroundServiceStopBehavior)
{
case BackgroundServiceStopBehavior.Sequential:
case BackgroundServiceStopBehavior.Sequential:
foreach (IHostedService hostedService in hostedServices)
{
try
{
await hostedService.StopAsync(token).ConfigureAwait(false);
Expand All @@ -137,24 +139,21 @@ public async Task StopAsync(CancellationToken cancellationToken = default)
{
exceptions.Add(ex);
}
break;
case BackgroundServiceStopBehavior.Asynchronous:
default:
tasks.Enqueue(hostedService.StopAsync(token));
break;
}
}

foreach (Task task in tasks)
{
try
{
await task.ConfigureAwait(false);
}
catch (Exception ex)
{
exceptions.Add(ex);
}
}
break;
case BackgroundServiceStopBehavior.Asynchronous:
default:
Task tasks = Task.WhenAll(hostedServices.Select(async service => await service.StopAsync(token).ConfigureAwait(false)));

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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,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 @@ -34,6 +36,85 @@ public async Task StopAsyncWithCancellation()
await host.StopAsync(cts.Token);
}

public static IEnumerable<object[]> StopAsync_ValidBackgroundServiceStopBehavior_TestCases
{
get
{
foreach (BackgroundServiceStopBehavior stopBehavior in Enum.GetValues(typeof(BackgroundServiceStopBehavior)))
{
foreach (int hostedServiceCount in new[] { 0, 1, 10 })
{
yield return new object[] { stopBehavior, hostedServiceCount };
}
}
}
}
[Theory]
[MemberData(nameof(StopAsync_ValidBackgroundServiceStopBehavior_TestCases))]
public async Task StopAsync_ValidBackgroundServiceStopBehavior(BackgroundServiceStopBehavior stopBehavior, int hostedServiceCount)
{
var callOrder = hostedServiceCount;
var hostedServices = new Mock<BackgroundService>[hostedServiceCount];
var executionCount = 0;

for (int i = 0; i < hostedServiceCount; i++)
{
var index = i;
var service = new Mock<BackgroundService>();
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, --callOrder);
})
.Returns(async () =>
{
if (stopBehavior.Equals(BackgroundServiceStopBehavior.Asynchronous) && hostedServiceCount > 1)
{
// Basically this will force all IHostedService instances to have StopAsync called before any one completes
executionCount++;
int waitCount = 0;

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

// This will ensure that we truly are stopping all services asynchronously
Assert.Equal(hostedServiceCount, executionCount);
}
})
.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>("backgroundServiceStopBehavior", stopBehavior.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();

await host.StopAsync(CancellationToken.None);

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

[Fact]
public void CreateDefaultBuilder_IncludesContentRootByDefault()
{
Expand Down