Skip to main content
replaced http://stackoverflow.com/ with https://stackoverflow.com/
Source Link

You would also need to use a workaround for the fact that you can't use await in a catch block (at least not until C# 6.0 comes out).

You would also need to use a workaround for the fact that you can't use await in a catch block (at least not until C# 6.0 comes out).

Source Link
svick
  • 24.3k
  • 4
  • 52
  • 89

First, threading bugs:

The way you're accessing _jobs from multiple threads is not thread-safe! You need to ensure that whenever you're reading or writing to the list, it's under a lock (probably under a different lock than _sync, since you don't want to wait for all current jobs to execute before a call to AddJob() returns.


private bool _paused;

Since this field is also accessed from multiple threads, you should also use a lock when accessing it.


Other notes:

I wanted a task scheduler/doer that enables my application to schedule some tasks to be executed in a specified time but in the same order they were scheduled in […]

Except your code doesn't do that. List<T>.Sort() is documented as being unstable, which means the order of jobs with the same StartTime isn't maintained after sorting.

OrderBy from LINQ is stable, though using that would mean creating lots of garbage lists.


[…] or depending on their priorities (DataFlow like).

I don't understand what this means, there are no priorities in TPL Dataflow.


private void ProcessJobs(object state)

At first I was confused by the parameter, until I realized it has to be there to fulfill the signature required by Timer. But since you don't actually use it, maybe it would be better to remove it from here and instead use a lambda as the timer parameter?

new Timer(_ => ProcessJobs(), null, …);

IJob[] jobsToExecute = (from jobs in _jobs
    where jobs.StartTime <= DateTimeOffset.Now && jobs.Enabled
    orderby jobs.Priority
    select jobs).ToArray();

Since _jobs is always kept sorted by StartTime, you don't need to iterate the whole list to get the jobs that should have been already started. So, using TakeWhile() instead of Where() is likely going to be more efficient (though it requires method syntax, so I switched to that for the whole query):

IJob[] jobsToExecute = _jobs
    .TakeWhile(job => job.StartTime <= DateTimeOffset.Now)
    .Where(job => job.Enabled)
    .OrderBy(job => job.Priority)
    .ToArray();

JobTriggered(job).Wait(_executionTimeOut);

I don't quite understand why are the job-handling methods async, when you're waiting on them synchronously: doing this doesn't save you any threads. Though you probably won't care if this uses few more threads than it needs to, since this class wouldn't make much sense in ASP.NET (you can't rely on it there, because the AppDomain can be recycled at any time when there is no request being processed).

If you wanted to make this fully asynchronous, you could do something like this to implement asynchronous waiting with timeout:

async Task<bool> WithTimeout(this Task task, TimeSpan timeout)
{
    var completedTask = await Task.WhenAny(task, Task.Delay(timeout));

    return completedTask == task;
}

…

await JobTriggered(job).WithTimeout(_executionTimeOut);

Though doing this would also mean you can't use lock and so you would have to use async-compatible lock (either SemaphoreSlim(1) or AsyncLock from Nito AsyncEx).

You would also need to use a workaround for the fact that you can't use await in a catch block (at least not until C# 6.0 comes out).


I'm not sure why are you using lock, when I think the way you're using the Timer means the method will never be called twice at the same time. If it's there just to make absolutely sure that doesn't happen, then I guess it makes sense.


long dueTime = Math.Min(Math.Max(100, (long)delay.TotalMilliseconds), (int)_baseInterval.TotalMilliseconds);
_timer.Change((int)dueTime, Timeout.Infinite);

The way you're using long here is weird. I always prefer to use TimeSpans as much as possible, not milliseconds in numeric types. There is no Math.Min() and Math.Max() for TimeSpan, but you can easily write them yourself:

public static class DateMath
{
    public static TimeSpan Min(TimeSpan val1, TimeSpan val2)
    {
        return new TimeSpan(Math.Min(val1.Ticks, val2.Ticks));
    }

    public static TimeSpan Max(TimeSpan val1, TimeSpan val2)
    {
        return new TimeSpan(Math.Max(val1.Ticks, val2.Ticks));
    }
}

This would change your code to the following, which I believe is more readable and less error prone:

var dueTime = DateMath.Min(DateMath.Max(TimeSpan.FromMillisedonds(100), delay), _baseInterval);
_timer.Change(dueTime, Timeout.InfiniteTimeSpan);

It looks like you're sorting the whole list just to get the first job. If the list of jobs becomes so large that this becomes a problem for you, consider switch to a heap, which is more efficient.

Though this means the TakeWhile() optimization mentioned above wouldn't be that simple anymore.


The two overloads of AddJob() have lots of duplicated code. I think you could get rid of that by calling the non-generic AddJob() from the generic one.


int Priority { get; }

You really need to document here whether a higher priority is indicated by a lower or higher numerical value.


TaskCompletionSource<T> TaskCompletionSource { get; set; }
void Return(T result);

Why does TaskCompletionSource have a setter?

And why expose both TaskCompletionSource and Return()? Wouldn't one of them be enough?


private async void DoApplication()

async void methods should be avoided, and especially so in console applications. Instead, you should use async Task method and Wait() for it. Doing that can cause a problem in other contexts, but it's the right solution here.

Or you could use AsyncContext from Nito AsyncEx.


public override async Task JobTriggered(IJob job)

All the casting in this method smells of a bad design. Maybe JobTriggered() should be a method on IJob and not on scheduler?