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

Stop IHostedService instances asynchronously by default #68216

Conversation

jerryk414
Copy link
Contributor

@jerryk414 jerryk414 commented Apr 19, 2022

Fixes #68036

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Apr 19, 2022
@ghost
Copy link

ghost commented Apr 19, 2022

Tagging subscribers to this area: @dotnet/area-extensions-hosting
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes 68036

Author: jerryk414
Assignees: -
Labels:

area-Extensions-Hosting, community-contribution

Milestone: -

/// <remarks>
/// Defaults to <see cref="BackgroundServiceStopBehavior.Asynchronous"/>
/// </remarks>
public BackgroundServiceStopBehavior BackgroundServiceStopBehavior { get; set; } =
Copy link
Member

Choose a reason for hiding this comment

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

Public API changes need to go through https://aka.ms/api-review process.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We will need to add some tests for the new and old behavior.

I agree, but I just wanted to see if this solution is the route that would be preferred prior to spending time on tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Public API changes need to go through https://aka.ms/api-review process.

It looks like the issue still needs a sponsor.

Copy link
Member

Choose a reason for hiding this comment

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

Can you update the issue to follow the template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@eerhardt
Copy link
Member

We will need to add some tests for the new and old behavior.

switch (_options.BackgroundServiceStopBehavior)
{
case BackgroundServiceStopBehavior.Sequential:
try
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 thought about just creating a Func<Task, Task> for handling tasks here instead of duplicating the try/catch logic, or even doing something where you add the task to the queue without starting it, but i felt like this is more easily understandable for future maintenance.

@maryamariyan maryamariyan marked this pull request as draft April 26, 2022 21:24
BackgroundServiceStopBehavior = stopBehavior;
}

var backgroundServiceExceptionBehavior = configuration["backgroundServiceExceptionBehavior"];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

So the reason I added these was primarily for tests - there was already a method in place to have configuration values set and for those to be honored in the test when setting up an IHost.. and i thought.. why aren't these supported?

@jerryk414 jerryk414 marked this pull request as ready for review May 6, 2022 17:29
@eerhardt
Copy link
Member

I'm going to close this PR until we get approval for the proposed API. We can reopen it once the API is approved. See https://aka.ms/api-review#pull-requests for more info.

@eerhardt eerhardt closed this May 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jun 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Hosting community-contribution Indicates that the PR has been added by a community member
Projects
None yet
2 participants