4
\$\begingroup\$

I am writing a task scheduler (that will one day grow up to be a windows service when I look into them) that should pretty much loop infinitely until I tell the program to stop. But having while(true) seems incorrect. What is the correct way to implement the loop? Use a dummy boolean variable that the code doesn't actually set to false ever (or should it set to false in a catch whose try encompasses the whole inside of the loop?)

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace DataPump
{
    class TaskManager
    {
        // Tasks added externally on an instance of TaskManager
        public Dictionary<DatapumpTask, Task> Tasks = new Dictionary<DatapumpTask, Task>();

        public void Go()
        {
            Func<Task, bool> isBusy = task => task.Status == TaskStatus.Running || 
                                              task.Status == TaskStatus.WaitingToRun || 
                                              task.Status == TaskStatus.WaitingForActivation;

            //Run Loop  --- To Do move off the main thread
            while (true)
            {
                for (int t = 0; t < Tasks.Count; t++)
                {
                    KeyValuePair<DatapumpTask, Task> task = Tasks.ElementAt(t);

                    if (task.Key.NextRun < DateTime.Now && (task.Value == null || !isBusy(task.Value)))
                    {
                        Tasks[task.Key] = Task.Factory.StartNew(() => task.Key.Run());
                    }

                    if (task.Value != null && task.Value.IsFaulted)
                    {
                        task.Key.Log("Task faulted - something is wrong!" , LogType.Error);
                        Tasks[task.Key] = null;
                    }
                }
            }
        }

    }

Note DatapumpTask has a public void Run() method, a public void Log() method that logs to a text file and Run() will set its DateTime NextRun property correctly...

\$\endgroup\$
0
2
\$\begingroup\$

The question is a bit diffuse but I can give some hopefully useful feedback to help you move forward.

First of all I have to say that there are many libraries available for scheduling tasks. So be aware that you are reinventing the wheel, not to mention a very non-trivial wheel!

Threading is difficult, I recommend you have a look at http://www.albahari.com/threading/

Using while(true) is fine. But you should pause execution within the loop to avoid spinning cpu. One way would be to figure out when the next task is scheduled to run and Thread.Sleep until then. You also need to deal with the situation where a new task is added that is due before the call to Sleep returns.

Consider using a callback to deal with completed tasks and update the Log/Status.

The Tasks field is public so TaskManager is not thread safe. Consider making it private and adding synchronized methods to manipulate it. Also, the DataPumpTask objects used as keys are mutable. What happens if the client code reads/writes these objects while they are being manipulated by the TaskManager thread?

I wouldn't use a for loop to iterate a dictionary unless I needed the loop index variable for some reason. Try this instead:

foreach(KeyValuePair<DataPumpTask, Task> pair in Tasks)
{
    var dataPumpTask = pair.Key;
    var task = pair.Value;
    //do stuff
}
\$\endgroup\$
5
  • \$\begingroup\$ Thanks. I'll look into the thread sleep thing, tasks won't get added during run time, just rescheduled after they complete. Do you know of a library that does this sort of scheduling with a basic tutorial? If I make Tasks private and expose it's .Add() method as public, is that threadsafe or still not :/ either way, I will only ever call .Add() on it before I call Go() so if that's the case is the threadsafe thing still a big problem? Lastly, I had it as a foreach but it wouldn't let me alter the value, maybe this was because I was trying on the KeyValuePair, I'll try your way. \$\endgroup\$
    – Dan
    Oct 1 '14 at 14:28
  • \$\begingroup\$ No good using the foreach, get a runtime error about changing the collection, I assume it's the Tasks[dataPumpTask] = Task.Factory.StartNew(() => task.Key.Run()); line that's causing the problem... \$\endgroup\$
    – Dan
    Oct 2 '14 at 8:20
  • \$\begingroup\$ That's true. My bad, sorry. You could iterate the keys with foreach(var dataPumpTask in Tasks.Keys) \$\endgroup\$ Oct 2 '14 at 9:05
  • \$\begingroup\$ If you can guarantee that there is no access to the Tasks collection after Go() has been called then at least the dictionary itself is safe. But will the maintenance programmer or yourself after 6 months remember? IMO it's better to be explicit and enforce thread safety or at least document it \$\endgroup\$ Oct 2 '14 at 9:25
  • 1
    \$\begingroup\$ Quartz.NET is a very capable library: quartz-scheduler.net \$\endgroup\$ Oct 2 '14 at 9:25

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.