views:

391

answers:

5

I'm looking at implementing a "Heartbeat" process to do a lot of repeated cleanup tasks throughout the day.

This seemed like a good chance to use the Command pattern, so I have an interface that looks like:

   public interface ICommand
   {
       void Execute();
       bool IsReady();
   }

I've then created several tasks that I want to be run. Here is a basic example:

public class ProcessFilesCommand : ICommand
{
    private int secondsDelay;
    private DateTime? lastRunTime;

    public ProcessFilesCommand(int secondsDelay)
    {
        this.secondsDelay = secondsDelay;
    }

    public void Execute()
    {
        Console.WriteLine("Processing Pending Files...");
        Thread.Sleep(5000); // Simulate long running task
        lastRunTime = DateTime.Now;
    }

    public bool IsReady()
    {
        if (lastRunTime == null) return true;

        TimeSpan timeSinceLastRun = DateTime.Now.Subtract(lastRunTime.Value);
        return (timeSinceLastRun.TotalSeconds > secondsDelay);
    }

}

Finally, my console application runs in this loop looking for waiting tasks to add to the ThreadPool:

class Program
{
    static void Main(string[] args)
    {

        bool running = true;

        Queue<ICommand> taskList = new Queue<ICommand>();
        taskList.Enqueue(new ProcessFilesCommand(60)); // 1 minute interval
        taskList.Enqueue(new DeleteOrphanedFilesCommand(300)); // 5 minute interval

        while (running)
        {
            ICommand currentTask = taskList.Dequeue();
            if (currentTask.IsReady())
            {
                ThreadPool.QueueUserWorkItem(t => currentTask.Execute());
            }
            taskList.Enqueue(currentTask);
            Thread.Sleep(100);
        }

    }
}

I don't have much experience with multi-threading beyond some work I did in Operating Systems class. However, as far as I can tell none of my threads are accessing any shared state so they should be fine.

Does this seem like an "OK" design for what I want to do? Is there anything you would change?

A: 

running variable will need to be marked as volatile if its state is going to be changed by another thread.

As to the suitability, why not just use a Timer?

Mitch Wheat
I'd be more tempted to use a Timer as well, and set the Timer for when the next task is due (which you should be able to figure out). No point in checking your task every 1/10th of a second.
kyoryu
No it doesn't. The worse case scenario is that you go through an extra cycle of the loop. This isn't a big deal considering that it could happen anyways because of the race between flipping the flag and checking it.
Jonathan Allen
@Jonathan Allen: Without the volatile keyword, running might be cached in a register.
Mitch Wheat
Not even close. In this context it means "running" must always be loaded from memory before "taskList".
Jonathan Allen
+7  A: 

This is a great start. We've done a bunch of things like this recently so I can offer a few suggestions.

  1. Don't use thread pool for long running tasks. The thread pool is designed to run lots of tiny little tasks. If you're doing long running tasks, use a separate thread.

  2. Have the Main() routine keep track of when things ran and how long till each runs next. Instead of each command saying "yes I'm ready" or "no I'm not" which will be the same for each command, just have LastRun and Interval fields which Main() can then use to determine when each command needs to run.

  3. Don't use a Queue. While it may seem like a Queue type operation, since each command has it's own interval, it's really not a normal Queue. Instead put all the commands in a List and then sort the list by shortest time to next run. Sleep the thread until the first command is needed to run. Run that command. Resort the list by next command to run. Sleep. Repeat.

  4. Don't use multiple threads. If each command's interval is a minute or few minutes, you probably don't need to use threads at all. You can simplify by doing everything on the same thread.

  5. Error handling. This kind of thing needs extensive error handling to make sure a problem in one command doesn't make the whole loop fail, and so you can debug a problem when it occurs. You also may want to decide if a command should get immediately retried on error or wait until it's next scheduled run, or even delay it more than normal. You may also want to not log an error in a command if the error happens every time (an error in a command that runs often can easily create huge log files).

Sam
#1: I would like to add that you can literally run out of thread pool threads, causing your application to dead lock. This is why you never allow long running threads into your ThreadPool.
Jonathan Allen
+1 for the point about short running tasks for the thread pool.
Mitch Wheat
+1  A: 

I would make all your Command classes immutable to insure that you don't have to worry about changes to state.

LWoodyiii
The command classes posted already have fields that change so they can't be immutable as is.
Sam
@Sam Yep, I agree. But this can be redesigned to make the class immutable.
LWoodyiii
+4  A: 

Instead of writing everything from scratch, you could choose to build your application using a framework that handles all of the scheduling and threading for you. The open-source library NCron is designed for exactly this purpose, and it is very easy to use.

Define your job like this:

class MyFirstJob : CronJob
{
    public override void Execute()
    {
        // Put your logic here.
    }
}

And create a main entry point for your application including scheduling setup like this:

class Program
{
    static void Main(string[] args)
    {
        Bootstrap.Init(args, ServiceSetup);
    }

    static void ServiceSetup(SchedulingService service)
    {
        service.Hourly().Run<MyFirstJob>();
        service.Daily().Run<MySecondJob>();
    }
}

This is all the code you will need to write if you choose to go down this path. You also get the option to do more complex schedules or dependency injection if needed, and logging is included out-of-the-box.

Disclaimer: I am the lead programmer on NCron, so I might just be a tad biased! ;-)

Jørn Schou-Rode
This is a very good suggestion and so for NCron looks very nice! I think I will ultimately end up going this route. Do you think you can touch base on the code provided and other suggestions made about people rolling their own as well? You are in a unique position of experience.
Simucal
+1 by the way. Thanks for introducing me to NCron.
Simucal
A: 

Now a days 'Parallel Extensions' from microsoft should be the viable option to write concurrent code or doing any thread related tasks. It provides good abstraction on top of thread pool and system threads such that you need not to think in an imperative manner to get the task done.

In my opinion consider using it. By the way, your code is clean.

Thanks.

Anand Patel