-
Notifications
You must be signed in to change notification settings - Fork 5k
Make BackgroundService run ExecuteAsync as task #116283
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
Conversation
8cea933
to
3e545d3
Compare
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.
Pull Request Overview
This PR ensures that all of BackgroundService.ExecuteAsync
runs on a background thread to avoid blocking startup, and updates tests to validate the new asynchronous behavior, including support for fully synchronous ExecuteAsync
implementations.
- Wraps
ExecuteAsync
inTask.Run
withinStartAsync
to push all execution off the caller thread. - Updates existing tests to expect
TaskCanceledException
on immediate cancellation and renames/refactors them. - Adds new tests covering synchronous
ExecuteAsync
implementations and their cancellation/stop behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs | Wraps ExecuteAsync call in Task.Run , capturing the returned task. |
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/BackgroundServiceTests.cs | Renames/refactors tests and adds new tests for sync execution and stop. |
src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/BackgroundServiceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/BackgroundServiceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs
Outdated
Show resolved
Hide resolved
Should we expose a config workaround for this breaking change |
That was specifically covered in #36063 and the API proposal around it. The API to control this was rejected and as a rule we don't proactively add AppContext compat switches until we have a strong signal that we need them. The belief here is that most folks will prefer that their |
This comment was marked as outdated.
This comment was marked as outdated.
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/Internal/HostTests.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/BackgroundServiceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/BackgroundServiceTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs
Outdated
Show resolved
Hide resolved
…undServiceFullStartAsync
It's possible the ExecuteAsync task completes in the time between it was started and StartAsync returns. If so, we were returning it's task. This leads to non-deterministic error behavior of the StartAsync API as well as inconsistently honoring `BackgroundServiceExceptionBehavior` based on this race. Instead always return a completed task.
src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/Internal/HostTests.cs
Show resolved
Hide resolved
…ckgroundService.cs Co-authored-by: Stephen Toub <[email protected]>
Thank you for finally addressing this! I assume it will be part of v10 to be released in November. Given the number of years this was open, the large number of people involved, and the potentially breaking or complex changes required on working systems, please reopen the original issue. It was closed by a bot. There will probably be a need by some to discuss it after upgrading, and it will be very hard to find the correct place; it took me a long time to find that thread and understand the context and underlying issue. You will have many new threads and confusion because of it. Just keep the original one open, please. |
Fixes #36063
That issue requested that we move the entire
BackgroundService.ExecuteAsync
to the async part of the method rather than give the service a chance to do synchronous work before it's first await. In this wayBackgroundService
s will not hold up the start of the application and folks are free to implement ExecuteAsync as a completely blocking method.This is a breaking change, but one that was deemed acceptable.