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

BackgroundService that doesn't call base.StartAsync leads to NullReferenceException - .NET 6 breaking change #60131

Closed
davidni opened this issue Oct 7, 2021 · 4 comments · Fixed by #60139
Assignees
Labels
area-Extensions-Hosting in-pr There is an active PR which will close this issue when it is merged
Milestone

Comments

@davidni
Copy link
Contributor

davidni commented Oct 7, 2021

Description

.NET 6.0 introduced an undocumented breaking change. IHostedService startup is now special-cased for BackgroundService, where the Host takes a direct dependency on BackgroundService.ExecuteTask, which can be null in some cases that previously worked.

private async Task TryExecuteBackgroundServiceAsync(BackgroundService backgroundService)
{
try
{
await backgroundService.ExecuteTask.ConfigureAwait(false);

When BackgroundService.ExecuteTask is null (e.g. because the override for BackgroundService.StartAsync decided not to call the base impl), this throws a NullReferenceException and causes the Host to abort (instead of previous behavior where IHostedService.StartAsync was called and that was it.

Example BackgroundService impl that breaks

class MyBackgroundService : BackgroundService
{
    public override Task StartAsync(CancellationToken cancellation)
    {
        return Task.CompletedTask;
    }

    protected override Task ExecuteAsync(CancellationToken stoppingToken)
    {
        return Task.CompletedTask;
    }
}

Configuration

  • Which version of .NET is the code running on? .NET 6.0.100-rc.1.21463.6
  • What OS and version, and what distro if applicable? Windows 10, but issue is not platform-specific
  • What is the architecture (x64, x86, ARM, ARM64)? x64, but issue is not platform-specific
  • Do you know whether it is specific to that configuration? Not platform-specific

Regression?

Yes, this is a breaking change in .NET 6. Previous versions did not special-case BackgroundService, and instead treated all IHostedService's uniformly. The new behavior IMHO violates the Principle of Least Astonishment.

Other information

Breaking change was introduced in #42981

Expectations for this issue

Either fix the regression, or document it as a breaking change in https://docs.microsoft.com/en-us/dotnet/core/compatibility/6.0

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Hosting untriaged New issue has not been triaged by the area owner labels Oct 7, 2021
@ghost
Copy link

ghost commented Oct 7, 2021

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

Issue Details

Description

.NET 6.0 introduced a =n undocumented breaking change. IHostedService startup is now special-cased for BackgroundService, where the Host takes a direct dependency on BackgroundService.ExecuteTask, which can be null in some cases that previously worked.

private async Task TryExecuteBackgroundServiceAsync(BackgroundService backgroundService)
{
try
{
await backgroundService.ExecuteTask.ConfigureAwait(false);

When BackgroundService.ExecuteTask is null (e.g. because the override for BackgroundService.StartAsync decided not to call the base impl), this throws a NullReferenceException and causes the Host to abort (instead of previous behavior where IHostedService.StartAsync was called and that was it.

Example BackgroundService impl that breaks

class MyBackgroundService : BackgroundService
{
    public override Task StartAsync(CancellationToken cancellation)
    {
        return Task.CompletedTask;
    }

    protected override Task ExecuteAsync(CancellationToken stoppingToken)
    {
        return Task.CompletedTask;
    }
}

Configuration

  • Which version of .NET is the code running on? .NET 6.0.100-rc.1.21463.6
  • What OS and version, and what distro if applicable? Windows 10, but issue is not platform-specific
  • What is the architecture (x64, x86, ARM, ARM64)? x64, but issue is not platform-specific
  • Do you know whether it is specific to that configuration? Not platform-specific

Regression?

Yes, this is a breaking change in .NET 6. Previous versions did not special-case BackgroundService, and instead treated all IHostedService's unifrmly. The new behavior IMHO violates the Principle of Least Astonishment.

Other information

Breaking change was introduced in #42981

Expectations for this issue

Either fix the regression, or document it as a breaking change in https://docs.microsoft.com/en-us/dotnet/core/compatibility/6.0

Author: davidni
Assignees: -
Labels:

untriaged, area-Extensions-Hosting

Milestone: -

@eerhardt
Copy link
Member

eerhardt commented Oct 7, 2021

I think we can fix the NRE in this scenario for 6.0.

@eerhardt eerhardt added this to the 6.0.0 milestone Oct 7, 2021
@eerhardt eerhardt self-assigned this Oct 7, 2021
@eerhardt eerhardt removed the untriaged New issue has not been triaged by the area owner label Oct 7, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Oct 7, 2021
eerhardt added a commit to eerhardt/runtime that referenced this issue Oct 7, 2021
In some scenarios, derived classes might not have called base.StartAsync, and ExecuteTask will still be null. Ensure we don't fail in those cases.

Fix dotnet#60131
eerhardt added a commit that referenced this issue Oct 8, 2021
* Check BackgroundService.ExecuteTask for null

In some scenarios, derived classes might not have called base.StartAsync, and ExecuteTask will still be null. Ensure we don't fail in those cases.

Fix #60131
github-actions bot pushed a commit that referenced this issue Oct 8, 2021
In some scenarios, derived classes might not have called base.StartAsync, and ExecuteTask will still be null. Ensure we don't fail in those cases.

Fix #60131
@eerhardt
Copy link
Member

eerhardt commented Oct 8, 2021

Re-opening to backport to 6.0.

@eerhardt eerhardt reopened this Oct 8, 2021
Anipik pushed a commit that referenced this issue Oct 8, 2021
* Check BackgroundService.ExecuteTask for null

In some scenarios, derived classes might not have called base.StartAsync, and ExecuteTask will still be null. Ensure we don't fail in those cases.

Fix #60131

* PR feedback

Co-authored-by: Eric Erhardt <eric.erhardt@microsoft.com>
@eerhardt
Copy link
Member

Fixed in 6.0 with #60177.

@ghost ghost locked as resolved and limited conversation to collaborators Nov 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Hosting in-pr There is an active PR which will close this issue when it is merged
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants