Skip to content

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

Merged
eerhardt merged 17 commits intodotnet:mainfrom
IEvangelist:background-service-ex-behavior
Apr 16, 2021
Merged

Add the ability to stop the Host when one of its BackgroundService instances errors#50569
eerhardt merged 17 commits intodotnet:mainfrom
IEvangelist:background-service-ex-behavior

Conversation

@IEvangelist
Copy link
Member

@IEvangelist IEvangelist commented Apr 1, 2021 •
edited
Loading

  • 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

@ghost
Copy link

ghost commented Apr 1, 2021

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: -

davidfowl
davidfowl requested changes Apr 1, 2021
View reviewed changes
davidfowl
davidfowl reviewed Apr 1, 2021
View reviewed changes
davidfowl
davidfowl reviewed Apr 1, 2021
View reviewed changes
@IEvangelist IEvangelist requested a review from davidfowl April 5, 2021 20:00
eerhardt
eerhardt reviewed Apr 7, 2021
View reviewed changes
eerhardt
eerhardt reviewed Apr 7, 2021
View reviewed changes
eerhardt
eerhardt reviewed Apr 7, 2021
View reviewed changes
@IEvangelist
Moved enum out of abstractions. Encapsulate logging logic in extensio…
28ccffc 
…ns. Define new EventId. Simplify check, avoid null eval. Update triple dash comments per peer review.
@IEvangelist IEvangelist requested a review from eerhardt April 7, 2021 18:26
@IEvangelist IEvangelist requested a review from eerhardt April 13, 2021 11:55
@IEvangelist IEvangelist requested a review from eerhardt April 13, 2021 15:23
@IEvangelist IEvangelist requested a review from eerhardt April 13, 2021 17:49
@eerhardt
Copy link
Member

eerhardt commented Apr 13, 2021

Looks like the new test is failling.

eerhardt
eerhardt approved these changes Apr 14, 2021
View reviewed changes
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

davidfowl 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?

@IEvangelist
Copy link
Member Author

IEvangelist commented Apr 15, 2021 •
edited
Loading

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

davidfowl commented Apr 15, 2021

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 •
edited
Loading

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.

eerhardt
eerhardt approved these changes Apr 15, 2021
View reviewed changes
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?

davidfowl
davidfowl approved these changes Apr 15, 2021
View reviewed changes
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

eerhardt commented Apr 15, 2021

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

eerhardt commented Apr 15, 2021

Logged #51346 for the test failures.

@eerhardt
Copy link
Member

eerhardt commented Apr 16, 2021

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.

Reviewers

@davidfowl davidfowl davidfowl approved these changes

@eerhardt eerhardt eerhardt approved these changes

+1 more reviewer

@maryamariyan maryamariyan maryamariyan approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

6.0.0

Development

Successfully merging this pull request may close these issues.

Generic Host is not stopped when BackgroundService crashes

5 participants

@IEvangelist @eerhardt @davidfowl @maryamariyan @karelz