+972 52-548-6969
Rotten Scheduling
time to read 1 min | 159 words
“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?
Comments
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.
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.
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
Not using the OS functions (task scheduler here) is the worst case of NIH.
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
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.
It's wrong for so many reasons I don't even know where to start...
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
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.
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.
I cannot grasp you cannot see this one yourself..
Yet another second for Markus - Task Scheduler (or one of the others mentioned above) would be a much more robust solution.
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.
Additional gripes: hard-coded interval, why is timer public? and do you rename your callback whenever the interval changes???
This code in a web application is really bad, because probably the worker process gets recycled before the timer reach the 72 hours interval.
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.
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.
All of my points goes to @Markus Zywitza
public static, so anything can inadvertently or maliciously callTimer.Init()to reset the timer.ExecuteEvery72Hours, the process will go down and the 72hr interval will be corrupted.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
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.
If you reboot the server, the schedule will get screwed up.
If you reboot the server, the schedule will get screwed up.
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.
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.
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.
@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.
The comments RSS feed is showing up blank for this post.
Comment preview