Ayende @ Rahien

Matt commented on Rotten Scheduling

Mon, 14 May 2012 08:21:37 GMT

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

Patrick Huizinga commented on Rotten Scheduling

Mon, 14 May 2012 07:39:26 GMT

@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.

André Sørhus commented on Rotten Scheduling

Sun, 13 May 2012 16:09:27 GMT

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.

Dan commented on Rotten Scheduling

Sun, 13 May 2012 12:06:26 GMT

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.

Peter commented on Rotten Scheduling

Sat, 12 May 2012 18:43:02 GMT

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.

Dmitry commented on Rotten Scheduling

Sat, 12 May 2012 04:16:25 GMT

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

Dmitry commented on Rotten Scheduling

Sat, 12 May 2012 04:16:12 GMT

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

Phil Bolduc commented on Rotten Scheduling

Fri, 11 May 2012 21:00:18 GMT

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.

Alek Davis commented on Rotten Scheduling

Fri, 11 May 2012 17:19:50 GMT

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

John commented on Rotten Scheduling

Fri, 11 May 2012 17:00:47 GMT

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.

Ken Egozi commented on Rotten Scheduling

Fri, 11 May 2012 16:47:36 GMT

All of my points goes to @Markus Zywitza

James Curran commented on Rotten Scheduling

Fri, 11 May 2012 16:26:20 GMT

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.

Karep commented on Rotten Scheduling

Fri, 11 May 2012 16:22:06 GMT

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.

Gian Maria commented on Rotten Scheduling

Fri, 11 May 2012 15:27:35 GMT

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

Gene Hughson commented on Rotten Scheduling

Fri, 11 May 2012 14:49:08 GMT

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

Tim commented on Rotten Scheduling

Fri, 11 May 2012 14:25:51 GMT

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 commented on Rotten Scheduling

Fri, 11 May 2012 14:02:53 GMT

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

jmr commented on Rotten Scheduling

Fri, 11 May 2012 13:28:54 GMT

I cannot grasp you cannot see this one yourself..

Sean Glover commented on Rotten Scheduling

Fri, 11 May 2012 12:23:53 GMT

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.

Khalid Abuhakmeh commented on Rotten Scheduling

Fri, 11 May 2012 11:31:29 GMT

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.

Giorgi commented on Rotten Scheduling

Fri, 11 May 2012 11:01:10 GMT

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

Thomas Levesque commented on Rotten Scheduling

Fri, 11 May 2012 10:17:45 GMT

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

Jason Meckley commented on Rotten Scheduling

Fri, 11 May 2012 10:01:11 GMT

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.

Sean Hederman commented on Rotten Scheduling

Fri, 11 May 2012 09:45:03 GMT

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

Markus Zywitza commented on Rotten Scheduling

Fri, 11 May 2012 09:19:31 GMT

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

Palesz commented on Rotten Scheduling

Fri, 11 May 2012 09:19:18 GMT

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

Felice Pollano commented on Rotten Scheduling

Fri, 11 May 2012 09:18:31 GMT

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.

Andrew Harry commented on Rotten Scheduling

Sun, 06 May 2012 23:11:01 GMT

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.