Ensure BackgroundService invokes ExecuteAsync after start - #132241
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3dfd3a15-eb10-455b-8c1c-16ecd87fd841
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
@jeffhandley deferring to you on who best to review. |
There was a problem hiding this comment.
Pull request overview
This PR adjusts BackgroundService.StartAsync scheduling so that ExecuteAsync is still invoked even if the service is stopped/disposed immediately after start, and adds regression coverage to validate the behavior under deterministic thread-pool starvation.
Changes:
- Update
BackgroundService.StartAsyncto avoid using the stopping token as theTask.Runscheduling token. - Add a regression test that blocks the sole thread-pool worker and verifies
ExecuteAsyncstill runs exactly once when immediately stopped/disposed. - Update the pre-canceled
StartAsynctest to assertExecuteTaskis canceled andExecuteAsyncis not invoked.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Hosting.Abstractions/src/BackgroundService.cs | Changes how ExecuteAsync is scheduled/canceled to prevent cancellation from suppressing delegate invocation. |
| src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/BackgroundServiceTests.cs | Adds deterministic regression coverage for immediate stop/dispose and refines pre-canceled start assertions. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3dfd3a15-eb10-455b-8c1c-16ecd87fd841
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/BackgroundServiceTests.cs:203
ExecuteInvocation.GetResult()has no timeout, so ifExecuteAsyncis never invoked this test will hang the RemoteExecutor process rather than failing with an assertion. Add a bounded wait (reusingRemoteExecutor.FailWaitTimeoutMilliseconds) before reading the result.
(int invocationCount, int threadId, bool isThreadPoolThread, bool isCancellationRequested) =
service.ExecuteInvocation.GetAwaiter().GetResult();
Assert.Equal(1, invocationCount);
src/libraries/Microsoft.Extensions.Hosting/tests/UnitTests/BackgroundServiceTests.cs:163
- The RemoteExecutor child process can hang indefinitely if the regression is reintroduced (or if the child process stalls). Similar tests in this repo set
RemoteInvokeOptions.TimeOutso a hang fails fast and doesn't consume the full harness timeout.
This issue also appears on line 201 of the same file.
var options = new RemoteInvokeOptions();
options.StartInfo.EnvironmentVariables["DOTNET_ThreadPool_UseWindowsThreadPool"] = "0";
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
rosebyte
left a comment
There was a problem hiding this comment.
I'm not sure I follow the wording about preserving .NET 10 behaviour. Do you mean that this preserves only the .NET 10 change whereby all ExecuteAsync work runs asynchronously on a thread-pool thread? As I understand the file history, invocation itself has had three different behaviours:
-
Before .NET 10,
ExecuteAsyncwas called regardless of whether the cancellation token was already cancelled. -
In .NET 10, whether
ExecuteAsyncis called effectively depends on whether the scheduler wins the race with cancellation. -
With this PR,
ExecuteAsyncis called if and only if the cancellation token has not already been cancelled whenStartAsyncchecks it.
The PR makes sense to me, and I consider the resulting behaviour clearly superior, so I'm going to approve it. I only wonder if I miss anything in the PR descriptiion.
|
@rosebyte before moving this forward I want to bring #131249 (comment) here. He's got a fair concern and there is a trade-off we are making as a result of the PR. We are trading “sometimes silently never invoke the service” for "always invoke it, requiring the service to honor an already-canceled token.”. Thoughts on what you think is best? |
|
Passing |
|
/ba-g Known issue #132336 |
|
/backport to release/11.0-rc1 |
|
Started backporting to |
…r start (#132598) Backport of #132241 to release/11.0-rc1 /cc @steveisok ## Customer Impact - [ ] Customer reported - [ ] Found internally [Select one or both of the boxes. Describe how this issue impacts customers, citing the expected and actual behaviors and scope of the issue. If customer-reported, provide the issue number.] ## Regression - [ ] Yes - [ ] No [If yes, specify when the regression was introduced. Provide the PR or commit if known.] ## Testing [How was the fix verified? How was the issue missed previously? What tests were added?] ## Risk [High/Medium/Low. Justify the indication by mentioning how risks were measured and addressed.] **IMPORTANT**: If this backport is for a servicing release, please verify that: - For .NET 8 and .NET 9: The PR target branch is `release/X.0-staging`, not `release/X.0`. - For .NET 10+: The PR target branch is `release/X.0` (no `-staging` suffix). ## Package authoring no longer needed in .NET 9 **IMPORTANT**: Starting with .NET 9, you no longer need to edit a NuGet package's csproj to enable building and bump the version. Keep in mind that we still need package authoring in .NET 8 and older versions. Co-authored-by: Steve Pfister <steveisok@users.noreply.github.com> Copilot-Session: 3dfd3a15-eb10-455b-8c1c-16ecd87fd841
Fixes #131249.
BackgroundService.StartAsynccurrently passes its stopping token toTask.Runas the scheduling token. IfStopAsyncorDisposecancels that token before the queued delegate begins, the task transitions toCanceledwithout invokingExecuteAsync.This makes invocation timing-dependent: after a non-canceled start is accepted,
ExecuteAsyncmay or may not run depending on whether the thread pool dequeues it before cancellation.This change:
ExecuteAsyncwork runs asynchronously on a thread-pool thread.CancellationToken.Noneas theTask.Runscheduling token.ExecuteAsync, so an immediate stop or dispose invokes it with cancellation already requested.StartAsyncbehavior by explicitly assigningTask.FromCanceled(cancellationToken)toExecuteTaskwithout invokingExecuteAsync.Regression tests deterministically occupy the sole thread-pool worker and verify that immediate stop and dispose still invoke
ExecuteAsyncexactly once on a thread-pool thread with an already-canceled stopping token. Pre-canceled startup coverage verifies thatExecuteTaskis canceled andExecuteAsyncis not invoked.Note
This pull request description was generated with GitHub Copilot.