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

Add the ability to stop the Host when one of its BackgroundService instances errors #50569

Merged
merged 17 commits into from
Apr 16, 2021

Conversation

IEvangelist
Copy link
Member

@IEvangelist IEvangelist commented Apr 1, 2021

  • Created BackgroundServiceExceptionBehavior enum.
  • Added HostOptions.BackgroundServiceExceptionBehavior property.
    • Defaults to StopHost, which was not the original behavior.
  • Added corresponding logic in Host
    • Renamed HandleBackgroundException to TryStartBackgroundServiceAsync as it is more conditional and appended the "Async" suffix as it is Task returning
    • Moved the invocation of StartAsync to the TryStartBackgroundServiceAsync to ensure that situations where it immediately throws are correctly handled.
  • Added unit tests.
    • Cover default behavior Ignore explicitly
    • Cover StopHost which calls IApplicationLifetime.StopApplication(), thus triggering ApplicationStopping.

Fixes #43637

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

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.

@ghost
Copy link

ghost commented Apr 1, 2021

Tagging subscribers to this area: @eerhardt, @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Created BackgroundServiceExceptionBehavior enum.
  • Added HostOptions.BackgroundServiceExceptionBehavior.
    • Defaults to Ignore, which is the current behavior.
  • Added corresponding logic in Host
    • Renamed HandleBackgroundException to TryStartBackgroundServiceAsync as it is more conditional and appended the "Async" suffix as it is Task returning
    • Moved the invocation of StartAsync to the TryStartBackgroundServiceAsync to ensure that situations where it immediately throws are correctly handled.
  • Added unit tests.
    • Cover default behavior Ignore explicitly
    • Cover StopHost which calls Environment.FailFast -- needed to use RemoteExecutor (thanks @eerhardt for the pointer)

Fixes #43637

Author: IEvangelist
Assignees: -
Labels:

area-Extensions-Hosting, new-api-needs-documentation

Milestone: -

…ns. Define new EventId. Simplify check, avoid null eval. Update triple dash comments per peer review.
@eerhardt
Copy link
Member

Looks like the new test is failling.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

Assuming CI passes clean, I think this is ready to be merged.

Thanks for the good work here, @IEvangelist.

@davidfowl
Copy link
Member

The one question I have is the behavior around starting. Before if you threw an exception on start it would fail starting the host but it never called StopApplication since the application never started in the first place and the exception bubbled to the the host.StartAsync. What's the behavior here? Is it still the same?

@IEvangelist
Copy link
Member Author

IEvangelist commented Apr 15, 2021

The one question I have is the behavior around starting. Before if you threw an exception on start it would fail starting the host but it never called StopApplication since the application never started in the first place and the exception bubbled to the the host.StartAsync. What's the behavior here? Is it still the same?

@davidfowl - This does introduce a change in behavior:

  • When an exception is thrown from a BackgroundService it is either ignored (logged), or a call to StopApplication is made
    • The Exception is not bubbled up to host.StartAsync.
  • If an exception is thrown from an IHostedService it bubbles as it always has, in other words - no change in behavior here

We will need to write up a "breaking change" doc, per @eerhardt. With these changes, by default, we'll stop the host.

Before these changes, if the host started a BackgroundService that threw immediately on its StartAsync call - it would bubble up, just like with IHostedService, and it was only "handled" with the call to await backgroundService.ExecuteTask. Hope that makes sense.

@davidfowl
Copy link
Member

The Exception is not bubbled up to host.StartAsync.

I think that behavior is wrong. We should revert it.

@IEvangelist
Copy link
Member Author

IEvangelist commented Apr 15, 2021

The Exception is not bubbled up to host.StartAsync.

I think that behavior is wrong. We should revert it.

@davidfowl - In looking at this again - an Exception thrown from a BackgroundService was only ever bubbled up to host.StartAsync when it was thrown from:

// Fire IHostedService.Start
await hostedService.StartAsync(combinedCancellationToken).ConfigureAwait(false);

Otherwise it was just logged in HandleBackgroundException. I believe that this was probably extremely rare, and I wouldn't be surprised if most errors occurred in:

await backgroundService.ExecuteTask.ConfigureAwait(false);

Since it is possible for an error to occur on either call, I intentionally grouped these two together to capture both possible background service failure scenarios. Are you suggesting that we throw from within the TryStartBackgroundServiceAsync? If so, why bother calling StopApplication?

I'm still a bit confused on why we wouldn't call StopAsync in the catch clause in the TryStartBackgroundServiceAsync method.

Update in chatting this over with @eerhardt I have a better understanding now. Updating this now.

Copy link
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM. @davidfowl - any thoughts?

Copy link
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

Very nice!

@eerhardt
Copy link
Member

Test failures seem unrelated:

C:\h\w\A918093D\w\A6DD0962\e>"C:\h\w\A918093D\p\dotnet.exe" exec --runtimeconfig System.Linq.Expressions.Tests.runtimeconfig.json --depsfile System.Linq.Expressions.Tests.deps.json xunit.console.dll System.Linq.Expressions.Tests.dll -xml testResults.xml -nologo -nocolor -notrait category=IgnoreForCI -notrait category=OuterLoop -notrait category=failing  
  Discovering: System.Linq.Expressions.Tests (method display = ClassAndMethod, method display options = None)
  Discovered:  System.Linq.Expressions.Tests (found 5645 of 5657 test cases)
  Starting:    System.Linq.Expressions.Tests (parallel test collections = on, max threads = 2)
    System.Linq.Expressions.Tests.ConvertTests.ConvertFloatToNullableULongTest(useInterpreter: False) [FAIL]
      Assert.Equal() Failure
      Expected: 0
      Actual:   9223372036854775808
      Stack Trace:
        /_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(11459,0): at System.Linq.Expressions.Tests.ConvertTests.VerifyFloatToNullableULong(Single value, Boolean useInterpreter)
        /_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(3128,0): at System.Linq.Expressions.Tests.ConvertTests.ConvertFloatToNullableULongTest(Boolean useInterpreter)
    System.Linq.Expressions.Tests.ConvertTests.ConvertDoubleToNullableULongTest(useInterpreter: False) [FAIL]
      Assert.Equal() Failure
      Expected: 0
      Actual:   9223372036854775808
      Stack Trace:
        /_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(9533,0): at System.Linq.Expressions.Tests.ConvertTests.VerifyDoubleToNullableULong(Double value, Boolean useInterpreter)
        /_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(1686,0): at System.Linq.Expressions.Tests.ConvertTests.ConvertDoubleToNullableULongTest(Boolean useInterpreter)
    System.Linq.Expressions.Tests.ConvertTests.ConvertNullableDoubleToNullableULongTest(useInterpreter: False) [FAIL]
      Assert.Equal() Failure
      Expected: 0
      Actual:   9223372036854775808
      Stack Trace:
        /_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(9904,0): at System.Linq.Expressions.Tests.ConvertTests.VerifyNullableDoubleToNullableULong(Nullable`1 value, Boolean useInterpreter)
        /_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(1938,0): at System.Linq.Expressions.Tests.ConvertTests.ConvertNullableDoubleToNullableULongTest(Boolean useInterpreter)
    System.Linq.Expressions.Tests.ConvertTests.ConvertNullableFloatToNullableULongTest(useInterpreter: False) [FAIL]
      Assert.Equal() Failure
      Expected: 0
      Actual:   9223372036854775808
      Stack Trace:
        /_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(11830,0): at System.Linq.Expressions.Tests.ConvertTests.VerifyNullableFloatToNullableULong(Nullable`1 value, Boolean useInterpreter)
        /_/src/libraries/System.Linq.Expressions/tests/Convert/ConvertTests.cs(3380,0): at System.Linq.Expressions.Tests.ConvertTests.ConvertNullableFloatToNullableULongTest(Boolean useInterpreter)
  Finished:    System.Linq.Expressions.Tests
=== TEST EXECUTION SUMMARY ===
   System.Linq.Expressions.Tests  Total: 35230, Errors: 0, Failed: 4, Skipped: 0, Time: 29.861s

if this change broke System.Linq.Expressions, I'm going to quit programming because computers don't make sense any more.

This smells like a coreclr break.

@eerhardt
Copy link
Member

Logged #51346 for the test failures.

@eerhardt
Copy link
Member

Libraries test failures are #51346
Installer test failures are #51372

@eerhardt eerhardt merged commit bbe669c into dotnet:main Apr 16, 2021
@IEvangelist IEvangelist deleted the background-service-ex-behavior branch April 16, 2021 14:35
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generic Host is not stopped when BackgroundService crashes
5 participants