dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.25k stars 4.73k forks source link

BackgroundService blocked the execution of whole host #36063

Open vova-lantsov-dev opened 5 years ago

vova-lantsov-dev commented 5 years ago

Describe the bug

When I run the specific foreach cycle in BackgroundService-derived class, it blocks the whole host from starting. Even second hosted service doesn't start. When I comment foreach cycle, everything works as expected.

To Reproduce

TargetFramework: netcoreapp2.1 Version: 2.1.12

Use following hosted service: NotificationRunner.cs And singleton: NotificationService.cs

Expected behavior

BackgroundService.ExecuteAsync should work in background without blocking even if it has blocking code. As you can see in NotificationService class, cancellation of enumerator is based on IApplicationLifetime.ApplicationStopping, but anyway it shouldn't affect the host startup because BackgroundService is expected to run in background :)

Screenshots

When foreach cycle exists image Then execution is blocked on this cycle image

But when foreach cycle is commented image Then execution continues as expected image (But why twice?)

Additional context

.NET Core SDK (reflecting any global.json):
 Version:   3.0.100-preview7-012821
 Commit:    6348f1068a

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.17763
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\3.0.100-preview7-012821\

Host (useful for support):
  Version: 3.0.0-preview7-27912-14
  Commit:  4da6ee6450

.NET Core SDKs installed:
  2.1.700 [C:\Program Files\dotnet\sdk]
  2.1.701 [C:\Program Files\dotnet\sdk]
  2.1.801 [C:\Program Files\dotnet\sdk]
  2.2.300 [C:\Program Files\dotnet\sdk]
  2.2.301 [C:\Program Files\dotnet\sdk]
  2.2.401 [C:\Program Files\dotnet\sdk]
  3.0.100-preview7-012821 [C:\Program Files\dotnet\sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.All 2.1.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.All 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.All]
  Microsoft.AspNetCore.App 2.1.11 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.5 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 3.0.0-preview7.19365.7 [C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 2.1.11 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.1.12 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.5 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 2.2.6 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.NETCore.App 3.0.0-preview7-27912-14 [C:\Program Files\dotnet\shared\Microsoft.NETCore.App]
  Microsoft.WindowsDesktop.App 3.0.0-preview7-27912-14 [C:\Program Files\dotnet\shared\Microsoft.WindowsDesktop.App]
vova-lantsov-dev commented 5 years ago

Fixed by adding await Task.Yield() at the top of method

dcarr42 commented 5 years ago

Yeah the method shouldn't be blocking. Wrapping in a Task.Run will also dispatch.

rynowak commented 5 years ago

Reopening this so we can look at it. I'm glad that you solved your problem but we should try to solve this so others don't hit it.

rynowak commented 5 years ago

@anurse @Tratcher - thoughts?

It looks like if your run a bunch of CPU-bound code in ExecuteAsync then it will stall startup until that work reaches the first actual async point.

See: https://github.com/aspnet/Extensions/blob/master/src/Hosting/Abstractions/src/BackgroundService.cs#L33

In health checks I ended up inserting a Task.Yield to avoid this. We'd get an additional state machine in this case, 1 per service, but it seems like a worthwhile tradeoff.

vova-lantsov-dev commented 5 years ago

Async method executes synchronously till first await is reached (it means BackgroundService.ExecuteAsync won't return till await is called). So this problem is not related to ASP.NET Core, but to async compilation.

vova-lantsov-dev commented 5 years ago

But it would be great if you can solve this case

Tratcher commented 5 years ago

It's a trade off. There may be reasonable initialization code the service needs to run before letting the app continue. It's within the service's control how long it blocks.

kostya9 commented 5 years ago

I have also encountered this issue and solved it in the same way as @vova-lantsov-dev did. However, my main concern is that this behavior is not straightforward, would be nice if this was more predictable or documented somewhere

analogrelay commented 5 years ago

I think it's reasonable to dispatch ExecuteAsync to the thread-pool in StartAsync. If the user wants to perform initialization that must complete before letting the app continue, they can override StartAsync itself, right?

public class MyService: BackgroundService
{
    public override async Task StartAsync()
    {
        await InitializeAsync();
        await base.StartAsync();
    }

    public override async Task ExecuteAsync()
    {
        // Do background stuff...
    }
}

It's called BackgroundService. It seems odd that you can block the foreground in the default case. If we're not happy with overriding StartAsync we could add a new InitializeAsync that is called and awaited during StartAsync.

This would be a breaking change however, since existing services may depend on the initial synchronous work blocking the startup process.

davidfowl commented 5 years ago

Sorry I never did this change but enough people have hit it now I think we should just do it. https://github.com/aspnet/Extensions/tree/davidfowl/background-service

pholly commented 5 years ago

I just ran into this and was about to leave feedback on the docs. Glad I checked here first. The Worker template works because it awaits a Task.Delay. Change that to a Thread.Sleep, remove async keyword, and return a CompletedTask from ExecuteAsync and it can be easily reproduced. Execution is not returned to the caller so StartAsync never finishes and the host never finishes initialization so cancellationtoken does not work and any other HostedServices registered would never start.

pholly commented 5 years ago

@davidfowl I just looked at the BackgroundService changes in your branch - would it make more sense for the Host to Task.Run each hosted service instead of depending on the hosted service to do it?

davidfowl commented 5 years ago

I'm not a fan as it's wasteful, but if that many people run into it it might be worth doing. I'd honestly rather teach people that these things aren't running on dedicated threads so blocking them isn't the way to go.

analogrelay commented 4 years ago

Triage summary: The work here is to consider dispatching BackgroundService.ExecuteAsync.

drmcclelland commented 4 years ago

It was very helpful to learn from the updated docs that "No further services are started until ExecuteAsync becomes asynchronous, such as by calling await." docs

Is there a possibility of this being addressed in .NET Core 3.1?

ghost commented 4 years ago

This is how I resolved it in my case [as detailed in 17674 referenced above]:

I started from scratch, choosing to implement my own version of it using IHostedService. In StartAsync, I start a single-fire System.Threading Timer with a period of 10s [10 is a value I came up with after a few runs of the setup to see how much time things took to startup]. The Timer's callback method actually fires the rest of the service (the stuff that is in the ExecuteAsync portion of the BackgroundService.

This works quite well in my setup and actually sped up the app start time [compared to even using the Task.Yield() workaround as mentioned above].

alexrp commented 4 years ago

For what it's worth:

The argument that you can just override StartAsync to accomplish the same thing is basically true but there is a pretty notable drawback: Now, state that you used to be able to initialize and use locally in ExecuteAsync has to be promoted to mutable class state. This gets particularly ugly when NRTs are enabled.

This:

class MyService : BackgroundService
{
    protected override async Task ExecuteAsync(CancellationToken ct)
    {
        var state = ...;

        await Task.Yield();

        while (true)
        {
            Work(state);

            await Task.Delay(..., ct);
        }
    }
}

Turns into this:

class MyService : BackgroundService
{
    T? _state;

    public override Task StartAsync(CancellationToken ct)
    {
        _state = ...;

        return base.StartAsync(ct);
    }

    protected override async Task ExecuteAsync(CancellationToken ct)
    {
        while (true)
        {
            Work(_state!);

            await Task.Delay(..., ct);
        }
    }
}

This seems like a significant regression in code clarity to me.

There's also an argument to be made that, if the current behavior of ExecuteAsync is misleading, then so too is the StartAsync override in the example above.

The name BackgroundService is definitely misleading given the current behavior. But I also think that the current behavior is actually useful - I rely on it in two of my projects for sane synchronous initialization ordering. So, I hope that if this change does go through, an alternative class will be provided that retains the current behavior.

mhintzke commented 4 years ago

@davidfowl is your commit here still a true WIP or are you redacting your idea to go this route? I found the documentation here that references this issue a little confusing as it initially made me think that even using await at the top of ExecuteAsync() would block the other host intialization when it actually doesn't (due to the fact that its actually the issuer's NotificationService enumeration that was blocking on his BlockingCollection)

I believe any confusion would be mitigated by doing what you initially proposed in that commit and having the abstraction dispatch our executing Task, although I also see the benefits of teaching the community about how to properly use it as-is as well.

Dotnet-GitSync-Bot commented 4 years ago

I couldn't figure out the best area label to add to this issue. Please help me learn by adding exactly one area label.

HelloKitty commented 4 years ago

I noticed that hangs can now occur in later versions of ASP Core if you do a Task.Delay. Previous versions of ASP Core were not causing this issue but I don't have exact version information. Calling await Task.Delay(JobConfig.IntervalMilliseconds, cancellationToken); within public async Task StartAsync(CancellationToken cancellationToken) In ASP Core 3.1 now, unlike a previous version, blocks startup for some reason.

vova-lantsov-dev commented 4 years ago

@HelloKitty IHostedService.StartAsync always blocks the execution of the whole host. This is expected behaviour. Try to use BackgroundService.ExecuteAsync for non-blocking delayed tasks.

HelloKitty commented 4 years ago

@vova-lantsov-dev It appears so, but I do not think this was the case in a previous version of ASP Core. Maybe 2.1. This ran fine without hanging a year ago, anyway I have adopted BackgroundService.ExecuteAsync and now delay in there. Seems to work.

drewkill32 commented 4 years ago

This causes an issue when unit testing. If I mock a service to return a CompletedTask instead of an awaited task StartAsync never returns.

 [TestClass]
public class TestClass
{
        [TestMethod, TestCategory("UnitTest")]
        public async Task Background_Service_Should_Return_FromStartAsync()
        {

            var sut = new FakeService();

            await sut.StartAsync(Token);

            //code never reaches here because ExecuteAsync never awaits a task
            Assert.IsTrue(true);
        }
}
public class FakeService : BackgroundService
{
        protected override async Task ExecuteAsync(CancellationToken stoppingToken)
        {
            while (!stoppingToken.IsCancellationRequested)
            {
                await Task.CompletedTask;
            }
        }
}
BowserKingKoopa commented 4 years ago

Just ran into this. Fixed with await Task.Yield() at top of BackgroundService.ExecuteAsync. I think this is especially confusing because we don't call ExecuteAsync ourselves, the framework does. We don't even instantiate the background service ourselves (we're using services.AddHostedService<>()). So the fact that the instantiation of the service and the call to ExecuteAsync happen synchronously as part of the ASP.NET app startup is not obvious. In fact it's downright strange imho.

pierregaboriaud commented 4 years ago

Or if it doesn't work you can use Task.Run

protected override Task ExecuteAsync(CancellationToken stoppingToken) => Task.Run(async () =>
{
    while (!stoppingToken.IsCancellationRequested)
    {
        // Do async work
        await Task.Delay(2000, stoppingToken);
    }
});
maryamariyan commented 4 years ago

Since a workaround exists for this issue and it is not a regression it is not a critical fix for 5.0, (moving to 6.0).

pawepaw commented 4 years ago

Facing same problem that background tasks is blocking main thread with IAsyncEnumerable. That's probably due to IAsyncEnumerableimplementation https://docs.microsoft.com/en-us/archive/msdn-magazine/2019/november/csharp-iterating-with-async-enumerables-in-csharp-8#under-the-hood-of-async-iterators

roblapp commented 3 years ago

I have a background process that has long periods of time that are blocking. What is the appropriate setup to have it run during the lifecycle of my application? Adding an await Task.Delay at the beginning of ExecuteAsync? Or using Task.Run(() => {...}); in ExecuteAsync?

vova-lantsov-dev commented 3 years ago

@roblapp both should work for you

But you forgot about 'async': Task.Run(async () => ...)

Assam commented 3 years ago

Based on the above I think documentation should be updated. It seems below class is doing blocking operation on ExecuteAsync

public class GracePeriodManagerService : BackgroundService
{
    private readonly ILogger<GracePeriodManagerService> _logger;
    private readonly OrderingBackgroundSettings _settings;

    private readonly IEventBus _eventBus;

    public GracePeriodManagerService(IOptions<OrderingBackgroundSettings> settings,
                                     IEventBus eventBus,
                                     ILogger<GracePeriodManagerService> logger)
    {
        // Constructor's parameters validations...
    }

    protected override async Task ExecuteAsync(CancellationToken stoppingToken)
    {
        _logger.LogDebug($"GracePeriodManagerService is starting.");

        stoppingToken.Register(() =>
            _logger.LogDebug($" GracePeriod background task is stopping."));

        while (!stoppingToken.IsCancellationRequested)
        {
            _logger.LogDebug($"GracePeriod task doing background work.");

            // This eShopOnContainers method is querying a database table
            // and publishing events into the Event Bus (RabbitMQ / ServiceBus)
            CheckConfirmedGracePeriodOrders();

            await Task.Delay(_settings.CheckUpdateTime, stoppingToken);
        }

        _logger.LogDebug($"GracePeriod background task is stopping.");
    }

    .../...
}
manuelelucchi commented 3 years ago

I noticed this happens while implementing IHostedService manually but even if you use Task.Run or Task.Yield on the StartAsync it doesn't work. You need BackgroundService and ExecuteAsync

GabrieleVolpato commented 3 years ago

I'm having the same problem using an IAsyncEnumerable in the background thread. The operation shouldn't be blocking (it's an async Enumerable after all) but @pawepaw seems to be right about this. For now, the Task.Yield workaround will do it for me.

Goffen commented 2 years ago

Wow what a wierd behaviour from a "background service"

jonas-lomholdt commented 2 years ago

I had this issue yesterday. While implementing BackgroundService it worked without any Task.Yield(), but today it doesn't. Does anyone know how that is possible? Adding the await Task.Yield() at the top fixed it, but I don't understand how it could work without it. This is .NET 6.

rjgotten commented 2 years ago

While implementing BackgroundService it worked without any Task.Yield(), but today it doesn't. Does anyone know how that is possible?

Look at the implementation of BackgroundService.

StartAsync is a synchronous (!!) method which returns either the Task taken as result from ExecuteAsync, if said task IsCompleted; and otherwise returns a pre-completed Task.CompletedTask. It will take responsbility to stop the execute task in StopAsync by triggering the cancellation token, regardless.

Because StartAsync is synchronous, the code will block until the actual ExecuteAsync implementation, if it is an async method, does its first await. That first await is where the asynchronous state machine compiler magic effectively kicks in and the async method returns its Task in a yet-to-be-completed state.

MartinDemberger commented 2 years ago

Another problem with this behaviour is with exceptions.

When the service throws a exception in the first line the process is stopped even if HostOptions.BackgroundServiceExceptionBehavior is set to BackgroundServiceExceptionBehavior.Ignore.

   public class ServiceWithError : BackgroundService
    {
        //protected override Task ExecuteAsync(CancellationToken stoppingToken) => Task.Run(() => throw new System.NotImplementedException());
        protected override Task ExecuteAsync(CancellationToken stoppingToken) => throw new System.NotImplementedException();
    }
eerhardt commented 2 years ago

This issue hasn't been planned for 7.0 (see https://github.com/dotnet/runtime/issues/64015). Moving to Future.

suchoss commented 1 year ago

Async method executes synchronously till first await is reached (it means BackgroundService.ExecuteAsync won't return till await is called). So this problem is not related to ASP.NET Core, but to async compilation.

I would like to point out that the "executes synchronously till first await is reached" is not true in my case.

protected override async Task ExecuteAsync(CancellationToken stoppingToken)
    {
        using var timer = new PeriodicTimer(TimeSpan.FromHours(24));

        // If I do not add following line, the code stops for very long time. More than 1 hour.
        await Task.Yield();  

        do
        {
            logger.Debug($"Timer triggered.");
            var count = Interlocked.Increment(ref executionCount);
            logger.Debug($"Trigger counter:{count}.");

            // this await wont return control back to main program
            // it is still blocking
            await RunLoop(stoppingToken);

        } while (await timer.WaitForNextTickAsync(stoppingToken));
    }

    private async Task RunLoop(CancellationToken stoppingToken)
    {
        foreach (var indexType in Enum.GetValues<AutocompleteIndexType>())
        {
            logger.Debug($"Creating {indexType:G} index.");
            try
            {
                // this await wont return control back to main program
                // it is still blocking - probably because 90 % of following calls are "synchronous" inside
                await IndexFactory.CreateCacheIndex(indexType);
            }
            catch (Exception e)
            {
                logger.Error($"Error occured during {indexType:G} index creation.", e);
            }
        }

        logger.Debug("Running delete of old indexes.");
        try
        {
            IndexFactory.DeleteOld();

        }
        catch (Exception e)
        {
            logger.Error($"Problem occured when deleting old files.", e);
        }
    }

But await Task.Yield(); at the beginning works.

Leandro-Albano commented 1 year ago

I just ran into this issue, and in my case it seems that await Task.Yield(); doesn’t help. In my case, it also seem to be a problem just on Debug mode. I have the following:

public class ConsumerBackgroundService : BackgroundService
    {
        protected override async Task ExecuteAsync(CancellationToken cancellationToken)
        {
            await Task.Yield();
            while (!cancellationToken.IsCancellationRequested)
            {
            }
        }

        public override Task StopAsync(CancellationToken cancellationToken) => base.StopAsync(cancellationToken);

    }
Leandro-Albano commented 1 year ago

So, I tried the above code, and apparently the issue only happens on Mac, on both vscode and vs for Mac.

thomrad commented 1 year ago

So, I tried the above code, and apparently the issue only happens on Mac, on both vscode and vs for Mac.

Yes, can confirm this message. Debug build in Rider on Mac blocks, Run in Rider on Mac works.

montella1507 commented 1 year ago

We have simple Backround service like this:

`

public class RefresherService : BackgroundService
{

    protected override async Task ExecuteAsync(CancellationToken stoppingToken)
    {
        try
        {

            while (!stoppingToken.IsCancellationRequested)
            {
                try
                {

                  await Do();
                }
                catch (Exception ex)
                {

                }
                finally
                {

                    await Task.Delay(TimeSpan.FromMinutes(1), stoppingToken);
                }
            }
        }
        catch (Exception ex) when (ex is TaskCanceledException || ex is OperationCanceledException)
        {

        }
        catch (Exception ex)
        {

        }
    }

}`

And it B L O C K S the startup of the server for 1 minute.... This is not how should have the BackgroundService and ExecuteAsync behave...

I have found article about this strange behavior here: https://blog.stephencleary.com/2020/05/backgroundservice-gotcha-startup.html

but.. somehow this important issue died?

Davilink commented 1 year ago

Maybe we should have 2 class of background service: BlockingBackgroundService and NonBlockingBackgroudService BlockingBackgroundService would use the same current implement of backgroundService and the NonBlockingBackgroudService would spawn internally spawn a Task.Run() just like demonstrated in this commit : https://github.com/dotnet/extensions/commit/8d0b1284ea8985eef7d52c0044f78e7c2fc26052#diff-a2481caefbc8935408d8a841d3040f716cac00cbd846ef84c9892ce4046c59d9R37

radwansalah commented 1 year ago

I have the same issue, but I noticed that this issue happens only when I run the project using the command line using "dotnet run", but when using the visual studio code or visual studio, it runs successfully, and the background service is working alongside the API, I test the API using postman and it works. Can anyone tell me how to run it using the command line successfully? For example, If I have a missing option something like that.

christallire commented 1 year ago

I've run into this too. @eerhardt could we make this happen in 8.0? this could be a relatively simple fix compared to the time spent scratching their heads :)

Xor-el commented 1 year ago

I think this should make it in NET 8.0 as indicated here https://github.com/dotnet/runtime/issues/86511 We have gotten two new properties added that should resolve this.

https://learn.microsoft.com/dotnet/api/microsoft.extensions.hosting.hostoptions.servicesstartconcurrently?view=dotnet-plat-ext-8.0

https://learn.microsoft.com/dotnet/api/microsoft.extensions.hosting.hostoptions.servicesstopconcurrently?view=dotnet-plat-ext-8.0

StephenCleary commented 1 year ago

The IHostedLifecycleService and ServicesStartConcurrently changes in .NET 8.0 do not address this issue. The host (as currently written) still starts the services by calling StartAsync directly. So a synchronous implementation of StartAsync will still block host startup.

lonix1 commented 1 year ago

Sad that even in v8 this won't be fixed.

For anyone who lands here, see Stephen Cleary's blog series which provides workarounds for these issues.

naumenkoff commented 11 months ago

.NET 8 has been released, and unfortunately, Microsoft has turned a blind eye to this issue. For those who have encountered it, it would be wise not to use BackgroundService. Instead, you can create your implementation based on IHostedService or the new IHostedLifecycleService.

The situation is amusing - to start a synchronous service, you need to await something, for example, Task.Yield, resulting in the generation of a state machine and a loss of at least 0.05% performance at this stage. Alternatively, you can use await Task.Run and lose at least 0.10% simply because of the presence of a state machine to start the service elegantly with just one addition to the DI container.

montella1507 commented 11 months ago

Yeah.. MS is way more focusing on implementing YET next type of UI library, just to deprecate and kill it 1 year later, rather then solve these issues..