patterncsharpMinor
Avoid while(true) in task scheduling code
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
Note
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:
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.