HangfireIO / Hangfire

An easy way to perform background job processing in .NET and .NET Core applications. No Windows Service or separate process required
https://www.hangfire.io
Other
9.41k stars 1.7k forks source link

Support async methods in IBackgroundJobClient interface #1658

Open sergey2048 opened 4 years ago

sergey2048 commented 4 years ago

Hi, Hangfire is a good tool to manage any type of background tasks. It allows to run distributed task asynchronously etc. But it is strange to see that methods which enqueue the background tasks work synchronously. Actually, all use cases imply to have a db storage to save the job state. Thus, an app server which uses Hangfire have to make a remote call to a db server and it looks like that in Hangfire those remote calls block treads of your app. Those calls could do their job very fast... but in case of network issues your high loaded server might be down pretty easily.

I'm working on a high load project and trying don't use any blocking calls. So it's a little bit confused to see how Hangfire works with its storage.

So the question is: Is there any reason why there are not any async methods in IBackgroundJobClient interface and its implementation. Is this by design or it it is complex ( or maybe very complex) to implement, taking in account a lot of storage implementations?

odinserj commented 4 years ago

All the preparations already made in 1.7.0 to support asynchronous background processes, and now there's a way to enable asynchronous APIs by changing every class starting from the top-level ones, and ending with bottom-level, and this is mostly a mechanical work. I've already tried to do this, and stopped for a while due to the following reason.

I dream to add async API support without introducing any breaking changes, but looks like there's no maintainable way to perform this. Async support is like ~corona~virus, and requires almost every class to be modified, but it's really hard to avoid code duplicates, see for example Run and RunAsync methods from the BackgroundExecution class.

Such duplicates will be everywhere in the code base if we follow the non-breaking way making everything non-maintainable, require unit test duplication, so I've planned this feature to the Hangfire 2.0 release some years ago... and started to delay the release due to the following reason.

First of all, I've seen a lot of issues in storage clients like System.Data.SqlClient and StackExchange.Redis related to the async pipeline, but unfortunately I don't have any links out of my head. And when I started to think about async APIs, there even were no async methods related to transactions some years ago – they were added only in .NET Core 3.0. So it was better to wait again, because bugs are fixed and new methods are added meanwhile.

A lot of codebases are still synchronous, and dropping it is a non-easy decision – not everyone is ready to upgrade to the pure asynchronous APIs, and sync-over-async doesn't work well in some scenarios and may lead even to deadlocks on some older platforms and exotic configurations. Although deadlocks could be solved by applying the ConfigureAwait(false) method, we should be careful enough, because if it's applied to every method, we'll get execution leaks from our dedicated threads to CLR's thread pool. But once this careful method is applied, we can even provide our own "sync-over-async" overloads for the top-level classes.

Also at least net45 and netstandard1.X platforms should be dropped, and some other older non-LTS platforms, and this decision should also be considered carefully.

And the main reason for the absence of async APIs is lack of traction on this problem. So everyone who's interested in this feature, please add the :+1: reaction to the first post of this thread. This is a big change to every class and produce compilation issues in end projects.

nikolai-mb commented 7 months ago

Are there any updates available regarding this? Would be great having a way to asynchronously schedule (and perhaps acquire distributed locks) in a asynchronous fashion. Bumping this since it's now the top-voted issue in the repo by a large margin

tuyen-vuduc commented 4 months ago

I faced a similar issue relating async tasks. Many HTTP calls in the background were failed due to unexpected IO corruption.

Is the async task enqueued and handled correctly?

BackgroundJob.Enqueue<IScheduler>(x => x.DoJobAsync(null, CancellationToken.None))
bartgommersMEH commented 2 months ago

Hangfire not properly/fully supporting async is a huge issue for us. All of our code is async. People/company still stuck with sync code bases will likely not update Hangfire anyway so in my opinion it's time (nay, way past due) to bite the bullet and add proper/full async support. We've ran in to multiple thread starvation issues because of Hangfire's lack of full async support. My company has a hangfire pro subscription, but we're seriously considering moving away from Hangfire in new projects because of it's lack of async support.

burningice2866 commented 2 months ago

DoJobAsync

Hangfire should handle it just fine if your method DoJobAsync returns a Task. You can see the specific implementation here and how it handles invoking Task/Async methods diffently.

https://github.com/HangfireIO/Hangfire/blob/43c54567113702f621f17d4616776e623ca648a2/src/Hangfire.Core/Server/CoreBackgroundJobPerformer.cs#L120-L128

odinserj commented 2 months ago

Unfortunately the same problems are still show stoppers for the full async implementation. Like @burningice2866 told, async background job methods are working fine and as expected, but regarding IBackgroundJobClient and other interfaces, there are problems.

I think the biggest problem relates to SQL clients that still don't support asynchronous transactions. Hangfire relies heavily of them to prevent leaks due to consistency problems. But async methods like CommitAsync, BeginTransactionAsync and so on are simply calling their synchronous variants, and nether System.Data.SqlClient, nor Microsoft.Data.SqlClient don't override them, leaving synchronous execution in the most sensitive execution path.

Another big problem is the absence of tooling that will help to diagnose "background job stuck" issues. Currently stack trace dump via stdump helps to reveal the majority of them, and for synchronous methods its implementation via Microsoft.Diagnostics.Runtime is pretty simple (but not during its major version bumps), and for async methods more complex implementation is required, since we'd need to walk over GC heap, parse Task instance internals, fetch method names, and so on. Without such tooling, it's very difficult to understand what's going on in critical cases.