-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Add the ability to start and stop IHostedService instances concurrently #75894
Conversation
…ged mid-execution; add support for stop behavior and exception behavior to be set in config file
…called "concurrently" or sequentially
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
Tagging subscribers to this area: @dotnet/area-extensions-hosting Issue DetailsImplements #68036
|
I'm not sure why the check It looks like it's failing on the Send to Helix step. I did find a helix log that indicates that the issue is unrelated to my changes but related to an issue with uploading helix logs. Seems like this could be related to #74699. Any assistance would be appreciated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution, @jerryk414!
@@ -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 |
There was a problem hiding this comment.
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.
@@ -30,11 +40,25 @@ public class HostOptions | |||
internal void Initialize(IConfiguration configuration) | |||
{ | |||
var timeoutSeconds = configuration["shutdownTimeoutSeconds"]; | |||
if (!string.IsNullOrEmpty(timeoutSeconds) | |||
if (!string.IsNullOrWhiteSpace(timeoutSeconds) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
:
runtime/src/libraries/System.Private.CoreLib/src/System/String.cs
Lines 491 to 501 in f518c05
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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!string.IsNullOrWhiteSpace(servicesStartConcurrently) | |
if (!string.IsNullOrEmpty(servicesStartConcurrently) |
} | ||
|
||
var servicesStopConcurrently = configuration["servicesStopConcurrently"]; | ||
if (!string.IsNullOrWhiteSpace(servicesStopConcurrently) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!string.IsNullOrWhiteSpace(servicesStopConcurrently) | |
if (!string.IsNullOrEmpty(servicesStopConcurrently) |
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 }; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
{ | ||
foreach (IHostedService hostedService in _hostedServices.Reverse()) | ||
// Ensure hosted services are stopped in FILO order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Ensure hosted services are stopped in FILO order | |
// Ensure hosted services are stopped in LIFO order |
_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); |
There was a problem hiding this comment.
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
_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); |
There was a problem hiding this comment.
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
} | ||
catch (Exception ex) | ||
{ | ||
exceptions.AddRange(tasks.Exception?.InnerExceptions?.ToArray() ?? new[] { ex }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
IList<Exception> exceptions = new List<Exception>(); | ||
if (_hostedServices != null) // Started? | ||
List<Exception> exceptions = new List<Exception>(); | ||
if (_hostedServices?.Any() == true) // Started? |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
Thanks for the contribution @jerryk414 let me know if you have any blockers with addressing the remaining feedback. |
This pull request has been automatically marked |
Closing it we can consider opening when we get more feedback. |
Implements #68036