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

Async/await support #248

Closed
DixonDs opened this issue Mar 30, 2015 · 27 comments
Closed

Async/await support #248

DixonDs opened this issue Mar 30, 2015 · 27 comments
Milestone

Comments

@DixonDs
Copy link

DixonDs commented Mar 30, 2015

It would be great if Quartz.net supports asynchronous jobs.

Also, I've found the relevant thread from the mailing list: http://quartz.10975.n7.nabble.com/quartznet-3668-using-Tasks-in-quartz-jobs-td10753.html

@lahma lahma added this to the 3.0 milestone May 9, 2015
@lahma
Copy link
Member

lahma commented May 9, 2015

Thanks for the request, we can investigate the possibilities with 3.0 release.

@mleyb
Copy link

mleyb commented Jul 13, 2015

I'll second this - it would be great for Quartz to support async/await.

Do we have any idea on a timeline for the 3.0 release?

@lahma
Copy link
Member

lahma commented Jul 13, 2015

I've started the async/await overhaul work and it's quite an undertaking. I won't be giving a timeline so that I won't disappoint you, but I'll keep you posted. There will be alpha and beta releases for sure to test.

lahma added a commit that referenced this issue Jul 19, 2015
@lahma
Copy link
Member

lahma commented Jul 19, 2015

I've done some massive changes to APIs and internals to better support async/await with commit f78cf38 . This should offer better internal efficiency when it comes to running the Quartz infrastructure.

The actual jobs still stay problematic. If we want jobs to have async Task support it would also mean ditching the current thread pool usage. Worker threads cannot be re-entered correctly when await is in use - this would break guarantees of having max as many jobs running as there are thread pool threads running.

To support async/await on job level would mean relying on .NET thread pool and some bigger separation of execution flow. I'm a bit hesitant to do this change..

@mleyb
Copy link

mleyb commented Jul 20, 2015

Can you suggest am alternative pattern to enable me to invoke await-able operations within jobs, if this work isn't going to be undertaken?

@Jawvig
Copy link

Jawvig commented Jul 20, 2015

To talk specifically to the point about breaking the promise of no more jobs than there are threads in the thread pool: Isn't breaking that promise one of the arguments for doing this work in the first place? When there is a lot of I/O going on we don't want to tie up user mode threads waiting for I/O completion as it means we have lots of pre-emptive scheduling of threads for polling for I/O completion and exhaust thread limits despite doing no useful CPU work on them. So if we want to maximize CPU and I/O usage, why not allow more jobs than the max threadpool count, if doing so means we can leave many of those jobs waiting on I/O congestion and not trying up a thread.

@lahma
Copy link
Member

lahma commented Jul 20, 2015

@mleyb I haven't given up all hope yet, still fiddling around with implementation alternatives...

@Jawvig Mixing the Quartz thread pool and .NET CLR thread pool have some consequences that might be hard to communicate.

We could effectively allow starting thousands of jobs at the same time when we have thread pool size of 10. All jobs start and start async operations and so the worker thread is returned and new jobs can start.

We might need a special thread pool (TaskScheduler based) for async based work that will fire of the work but then it gets interesting. This leads me to think that this special thread pool should not allow configuration of size as "it depends".

I'd say this might be fine and dandy up until the point where CLR thread pool exhaustion occurs, say that it's an ASP.NET app that has requests queuing up. Quartz has been able to circumvent exhaustion by having its own thread pool and using sync jobs.

So I just wanted to raise these points that might be problematic and it's great that this discussion is happening. I eagerly want to hear all opinions about pros and cons on the matter.

Some food for thought:

  • should we have IAsyncJob interface, retire IInteruptableJob and have IJobExcutionContext contain CancellationToken that can be utilized from both async and sync jobs?
  • should we only allow IAsyncJobs running on scheduler that has CLR thread pool configured as the only thread pool - or should we run hybrid where sync jobs are run on thread pool and async jobs on top of that on their own?

@Jawvig
Copy link

Jawvig commented Jul 23, 2015

Re: retiring IInterruptableJob: Even though we are talking about a major release, I think we should keep IInterruptableJob for backwards compatibility. I cannot think of the circumstances under which cancelling via a CancellationToken and also calling Interrupt would be a problem. There's slight bloat to Quartz, but we could alter the code doc to indicate that IInterruptableJob might be removed in future releases and that use of the CancellationToken in the IJobExecutionContext is preferred.

Re: IAsyncJob interface: I think this is the correct thing to do.

Re: Running IAsyncJobs on a scheduler with hybrid threadpools: I get the impression this would be a bad idea. When the CLR is being relied on to provide a tuned ThreadPool on which TPL can perform its operations, it would seem wrong to undermine the assumptions it was built upon by having a set of extra threads sitting in the same process that it has no visibility of. So to be clear, I would be in favour of insisting on only allowing IAsyncJobs to run on a scheduler where the CLR thread pool is configured as the only thread pool.

@lahma
Copy link
Member

lahma commented Jul 26, 2015

I've done first iteration with the concept now, example 16 has code to introduce the IAsyncJob:

https://github.com/quartznet/quartznet/tree/quartznet-3/src/Quartz.Examples/example16

@mrfishface
Copy link

Wouldn't IJobAsync be the proper naming convention that would fall in line with other async pattern names?

On Jul 26, 2015, at 6:23 AM, Marko Lahma notifications@github.com wrote:

I've done first iteration with the concept now, example 16 has code to introduce the IAsyncJob:

https://github.com/quartznet/quartznet/tree/quartznet-3/src/Quartz.Examples/example16


Reply to this email directly or view it on GitHub.

@lahma
Copy link
Member

lahma commented Jul 27, 2015

I believe that the common pattern with async methods is to postfix method names with Async (that Quartz does not currently do). So I'd argue that with class name like AsyncJob has a better message about intent.

@vchekan
Copy link

vchekan commented Jul 27, 2015

@lahma

ASP.NET app that has requests queuing up. Quartz has been able to circumvent exhaustion by having its own thread pool and using sync jobs.

If I'm not mistaken, you are talking about "back-pressure". I've ran into this problem of async task being queued by very fast and practically infinite length message producer, which enqueued so many messages within a minute that process runs out of memory.

I used Reactive Extensions at that time, and replacing it with Tpl's Dataflow solved the problem perfectly. How about using ActionBlock:
https://msdn.microsoft.com/en-us/library/hh194684%28v=vs.110%29.aspx

var executor = new ActionBlock<IJobAsync>(async job => await job.ExecuteAsync(), 
    new ExecutionDataflowBlockOptions{
         MaxDegreeOfParallelism = maxTasksLimit,
         BoundedCapacity = 1
    });

 // scheduler
SchduleJob(job) {
     await executor.SendAsync(job)
  1. We can put a limit on simulteneously executing tasks.
  2. Executor's buffer capacity is bounded so if limit is reached, sender will "await".

@lahma
Copy link
Member

lahma commented Jul 28, 2015

@vchekan thanks! I need to study this a bit more.

@Jawvig
Copy link

Jawvig commented Jul 28, 2015

ActionBlock is pretty handy. I'm using it to implement throttling against a third party API.

To build on Vadim's comment, we would probably need to do more than await executor.SendAsync(job) in order to implement misfire strategies, etc. We could Task.Wait(TimeSpan) and treat that as a misfire if the timeout expires.

Anyway, aside from that, I wanted to comment on the API method names in the example you put up, Marko. As you noted, most async methods one encounters are suffixed "Async", whereas the ones in your example weren't. Furthermore, many methods will have become async without having changed name, e.g. IScheduler.Start, IScheduler.Interrupt. I think this might have too many unintended consequences on existing code on upgrade. Given that they are void methods, client code with still compile against it and sail through creating tasks without waiting on their completion.

May I suggest keeping the existing methods and adding new methods that end 'Async'? The existing methods can be implemented as synchronous methods that call the Async methods and .Wait() on their tasks. I have a feeling that this will provide a much easier upgrade path for people without unintended side effects.

Apologies if I'm missing something obvious about the intent, here!

@lahma
Copy link
Member

lahma commented Jul 28, 2015

@Jawvig I'm still a bit on the fence about using Async postfix. I don't see much point on having both sync and async versions of methods as async is now preferred. Having async-only public API then begs the question whether it makes sense to have Async postfix. Breaking the compilation is of course nice so that API consumer knows what to prepare for.

I pinged MassTransit project about this (they are undergoing same change) and had the discussion here MassTransit/MassTransit#314 .

There's a separate test for detecting that are no async void methods (which are the killers).

@DixonDs
Copy link
Author

DixonDs commented Dec 16, 2015

@lahma I don't really like to ping, but are there any plans or estimates when (or if) this going to be done and released into the wild?

@mleyb
Copy link

mleyb commented Dec 16, 2015

agreed - i'd really love to start using this

@Romanx
Copy link

Romanx commented Jan 14, 2016

👍 for some info :)

@lahma
Copy link
Member

lahma commented Apr 23, 2016

You can follow the progress of branch quartznet-3 https://github.com/quartznet/quartznet/tree/quartznet-3 , there are now nightly builds yet but API already has many changes to support the model.

lahma added a commit that referenced this issue Apr 24, 2016
This reverts commit e83ddfc.

# Conflicts:
#	src/Quartz.Tests.Unit/QualityTest.cs
#	src/Quartz/Core/JobRunShell.cs
#	src/Quartz/Core/QuartzSchedulerThread.cs
#	src/Quartz/IAsyncJob.cs
#	src/Quartz/IJobExecutionContext.cs
lahma added a commit that referenced this issue Apr 24, 2016
lahma added a commit that referenced this issue Apr 24, 2016
This reverts commit e83ddfc.

# Conflicts:
#	src/Quartz.Tests.Unit/QualityTest.cs
#	src/Quartz/Core/JobRunShell.cs
#	src/Quartz/Core/QuartzSchedulerThread.cs
#	src/Quartz/IAsyncJob.cs
#	src/Quartz/IJobExecutionContext.cs
@robborden
Copy link

Sorry to be a pest but just wondering if you have a timeline for when Quartz 3 will be ready. My project is hinging on the completion of this feature at the moment. It seems you cannot run an HttpClient request in a Quartz job at the moment if you need synchronous behavior and have an interval lower than the round trip time of the Http request. Quartz won't "await" the return value and your requests start overlapping. I assume this new async job type would resolve that issue? I'm using mono and I think HttpClient is the only option and it only has async requests.

@lahma
Copy link
Member

lahma commented Apr 29, 2016

Asyn jobs themselves might not be the silver bullet. You can force the synchronous result for HttpClient by using client.GetAsync(_address).Result which evaluates the request. Does that work for you? You also mentioned overlapping of jobs, that can be handled with [DisallowConcurrentExecution] if that helps.

And yes, I dodged the original question, as I don't want to give promises that would hard to keep.

@robborden
Copy link

I appreciate your fast (and honest) response. Can you give me a quarter where you currently are hoping to release by? Like Q3 2016, Q4 2016, Q1 2017?

I found a similar option to what you provided within 2 minutes of posting (always happens?). I already have it working fine, so no worries there.

I should take the opportunity to express my love for Quartz. I used it to build a microframework to facilitate configuring, writing, and deploying microservices and Quartz is the heart of it. I was lucky to come across it before I re-wrote something that was half as good. Thank you to all contributors for making it available.

@lahma
Copy link
Member

lahma commented Apr 30, 2016

I think we can start giving out alpha quality in next couple of weeks.The basics are solid (trigger calculations, job store data handling - they have not had changes) but as the work evolves around async/await and thread pools and API their stability cannot be guaranteed. Remoting will also be dropped and need to be rewritten on top of HTTP/Web API so remote controlling of scheduler will not be part of the first drops.

@bilal-fazlani
Copy link

Any update on this ?

@lahma
Copy link
Member

lahma commented Jul 14, 2016

Alpha build without remote management should be doable quite soon to nuget.org feed.

@lahma
Copy link
Member

lahma commented Aug 16, 2016

Alpha 1 has landed to NuGet, please see the announcement. A lot still to do but now should be easier to kick the tyres.

I'm closing this issue for now, thanks for your patience!

@lahma lahma closed this as completed Aug 16, 2016
@lahma lahma modified the milestones: 3.0-alpha1, 3.0 Aug 16, 2016
@mleyb
Copy link

mleyb commented Aug 17, 2016

Amazing scenes! Thanks - can't wait to try this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants