current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite
1

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...

share|improve this question

1 Answer 1

up vote 2 down vote accepted

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
}
share|improve this answer
    
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. –  Dan Oct 1 '14 at 14:28
    
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... –  Dan Oct 2 '14 at 8:20
    
That's true. My bad, sorry. You could iterate the keys with foreach(var dataPumpTask in Tasks.Keys) –  robertfriberg Oct 2 '14 at 9:05
    
Thanks I'll give that a try –  Dan Oct 2 '14 at 9:11
    
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 –  robertfriberg Oct 2 '14 at 9:25

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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