HiveBrain v1.2.0
Get Started
← Back to all entries
patterncsharpMinor

Avoid while(true) in task scheduling code

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
whileschedulingtrueavoidcodetask

Problem

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 Tasks = new Dictionary();

        public void Go()
        {
            Func 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  task = Tasks.ElementAt(t);

                    if (task.Key.NextRun  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...

Solution

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 pair in Tasks)
{
    var dataPumpTask = pair.Key;
    var task = pair.Value;
    //do stuff
}

Code Snippets

foreach(KeyValuePair<DataPumpTask, Task> pair in Tasks)
{
    var dataPumpTask = pair.Key;
    var task = pair.Value;
    //do stuff
}

Context

StackExchange Code Review Q#64354, answer score: 2

Revisions (0)

No revisions yet.