Ayende @ Rahien

It's a girl

Rotten Scheduling

“We need to run a specific task every 72 hours, I thought about this approach…”

public class TimedTask
{
  public static Timer Timer;
  
  public static void Init()
  {
    Timer = new Timer(ExecuteEvery72Hours, null, TimeSpan.FromHours(72), TimeSpan.FromHours(72));
  }
  
  public static void ExecuteEvery72Hours()
  {
    // do something important
  }
}

Let us assume that this code is being called properly. Why is this a bloody rotten idea?

Tags:

Posted By: Ayende Rahien

Published at

Originally posted at

Comments

Andrew Harry
05/06/2012 11:11 PM by
Andrew Harry

The first thought - how long does 'ExecuteEvery72Hours' take to run? If it is a significant amount of time, then it is going to delay the start of the next execution run.

Before long instead of being 3 days at 1am it will be plus execution time.

Second issue - how is the system to know when it was due to run next if it was rebooted? how criticial is it be accurate?

A scheduler worth it's salt needs to be reliable and run like clock work, not like a train time table.

Felice Pollano
05/11/2012 09:18 AM by
Felice Pollano

In 72 hours the machine running this code can be restarted maybe more than once for a lot of reason. So basically the task scheduling is not persisted and there is no strategy to guarantee the task will be executed at the proper time, since the execution tiome depends on when you start it. No reason to extend more, that code is a little to naive to go in any production environment.

Palesz
05/11/2012 09:19 AM by
Palesz

I agree with Harry. The task is running in every 72 hours if the application is running and not restarted. If it has been restarted, there will be delay's in the execution (and in special cases the task will never execute: restart the app in every 2 days).

Proper scheduling: starting DATE + period

Markus Zywitza
05/11/2012 09:19 AM by
Markus Zywitza

Not using the OS functions (task scheduler here) is the worst case of NIH.

Sean Hederman
05/11/2012 09:45 AM by
Sean Hederman

Absolutely agree with Markus. I don't care what the code looks like; your requirement is more than catered for by built in OS components. This one is one that a HUGE number of developers write themselves and shouldn't

Jason Meckley
05/11/2012 10:01 AM by
Jason Meckley

The component is responsible for the when and the how. These are separate concepts and shuold be separated. Otherwise there are all kinds of problems with timing (as mentioned above) and error handeling.

Thomas Levesque
05/11/2012 10:17 AM by
Thomas Levesque

It's wrong for so many reasons I don't even know where to start...

Giorgi
05/11/2012 11:01 AM by
Giorgi

For simple jobs windows task manager is enough but once you need more functionality Quartz .net is an excellent library with many features for managing jobs

Khalid Abuhakmeh
05/11/2012 11:31 AM by
Khalid Abuhakmeh

Oh this code looks familiar, I think I wrote something like this once. Then realized 72 hours from when?

You need some base line time to go 72 hours from or else it will not be very dependable due to application / server restarts.

Overall I agree with Giorgi. Use scheduler software like task scheduler, quartz.net, or event momentapp.com <-- web based scheduler.

Sean Glover
05/11/2012 12:23 PM by
Sean Glover

I second Markus. Lack of understanding of basic OS capabilities and community endorsed 3rd party tooling is no excuse to reinvent the wheel, especially with such a naive implementation.

Another relevant topic of discussion is the old polling vs event driven argument. Consider using a SOA pattern with a publisher-subscriber model and consume domain events as they occur instead of waiting for the next 72 hour window.

jmr
05/11/2012 01:28 PM by
jmr

I cannot grasp you cannot see this one yourself..

Gene Hughson
05/11/2012 02:02 PM by
Gene Hughson

Yet another second for Markus - Task Scheduler (or one of the others mentioned above) would be a much more robust solution.

Tim
05/11/2012 02:25 PM by
Tim

I was going to shout for Quartz.net too. You can persist your scheduled tasks to file, database or roll your own persister, but the scheduling is all sorted.

Gene Hughson
05/11/2012 02:49 PM by
Gene Hughson

Additional gripes: hard-coded interval, why is timer public? and do you rename your callback whenever the interval changes???

Gian Maria
05/11/2012 03:27 PM by
Gian Maria

This code in a web application is really bad, because probably the worker process gets recycled before the timer reach the 72 hours interval.

Karep
05/11/2012 04:22 PM by
Karep

To people talking about OS Task Scheduler. Is there some .NET wrapper for it? If not than I have no idea what the hell are you talking about.

James Curran
05/11/2012 04:26 PM by
James Curran

In addition to what those above said, this method also requires an app running 24x7, tying up, if not clock cycles, at least memory and other resources -- to handle a job that's already built into virtually every modern OS.

Ken Egozi
05/11/2012 04:47 PM by
Ken Egozi

All of my points goes to @Markus Zywitza

John
05/11/2012 05:00 PM by
John
  1. Everything is public static, so anything can inadvertently or maliciously call Timer.Init() to reset the timer.
  2. The process running the timer must run constantly and be perfectly stable. LOL. If the process hangs or crashes, scheduled tasks will be missed.
  3. If anything goes wrong in ExecuteEvery72Hours, the process will go down and the 72hr interval will be corrupted.
Alek Davis
05/11/2012 05:19 PM by
Alek Davis

The first series of questions I have are around the 72-hour execution. Why is the job supposed to run every 72 hours (once in three days)? Does it take roughly three days to complete? What if it does not complete in three days? Would you want to run it 72 hours after completion or 72 hours after the last start? What if it completes in half an hour? Would you want to run id daily then? Would it make sense to run the job on certain days of the week instead? Then there are all the questions that have been raised by others (i.e. what to do if the job fails, etc). Scheduled tasks are difficult to do right and unfortunately there re not many facilities in .NET to do this. I have to deal with the exact same issues a while back, and my approach was basically to have different type of jobs, such as daily job (runs once per day), weekly job (runs on certain days of the week), and timed job (runs every certain number of minutes or hours). There are a few caveats that I had to solve including not starting the job of previous run overlaps the next start time, failure to run, completion close to the next scheduled interval. I can't say that I have a bullet-proof approach, but if anyone needs to deal with a similar problem, take a look at the sample (it has all classes needed for various types of scheduled tasks in a windows service): http://goo.gl/iRO36

Phil Bolduc
05/11/2012 09:00 PM by
Phil Bolduc

The main problem is the drift introduced by using the Timer. Assuming there is always a thread available, my measurements show that there can be 10 to 250 millisecond delay between the time your callback is schduled.

Dmitry
05/12/2012 04:16 AM by
Dmitry

If you reboot the server, the schedule will get screwed up.

Dmitry
05/12/2012 04:16 AM by
Dmitry

If you reboot the server, the schedule will get screwed up.

Peter
05/12/2012 06:43 PM by
Peter

I found something similar in the Orchard codebase a while back, and though I think it's "rotten" if you want scheduled tasks, I think for them, given the alternative is running all these scheduled tasks during an HTTP request/response, I can see the justification.

They aren't doing the exact same thing though, so it may be an apples to oranges comparison. It's just the only place I've seen similar code ever.

Dan
05/13/2012 12:06 PM by
Dan

This looks horribly familiar.

I had to reverse engineer and refactor a bunch of Windows services originally written by someone called Pankaj Rathote that used Timer in precisely same way, except it was supposed to fire ONCE A WEEK.

No doubt there were issues.

André Sørhus
05/13/2012 04:09 PM by
André Sørhus

In addition to all of the above, and more than anything, this as a rotten implementation of a singleton pattern.

Each call to Init() will start a new timer and effectively orphan any existing one. (And it won't be cleaned up by the garbage collector.) So all the instances of Timer will trigger every 72nd hour, multiplying the calls through the TimerCallback delegate.

You should also pay attention to that System.Threading.Timer implements IDisposable, and need a more graceful finalizing.

Patrick Huizinga
05/14/2012 07:39 AM by
Patrick Huizinga

@Andrew Harry The first thought - how long does 'ExecuteEvery72Hours' take to run? If it is a significant amount of time, then it is going to delay the start of the next execution run.

Before long instead of being 3 days at 1am it will be plus execution time.

Nope, the System.Threading.Timer executes its callback on the threadpool. The timer will not be affected by the time the callback takes, so the next run will be one interval later, regardless of the time a previous callback took. See also the remarks section at http://msdn.microsoft.com/en-us/library/system.threading.timer.aspx. The callback can be executed simultaneously on two thread pool threads if the timer interval is less than the time required to execute the callback, [...]

@André Sørhus And [the timer] won't be cleaned up by the garbage collector.

Yes, the timer will be garbage collected. See also the remarks at http://msdn.microsoft.com/en-us/library/system.threading.timer.aspx. The fact that a Timer is still active does not prevent it from being collected.

Matt
05/14/2012 08:21 AM by
Matt

The comments RSS feed is showing up blank for this post.

Comments have been closed on this topic.