[Mono-devel-list] Reworked Patch of Timer

[Ben Maurer]

General issues: 
      * Tabs are still off. Something is funky with your editor ;-) 
      * Please follow the mono coding guidelines
        (http://mono-project.com/Coding_Guidelines) 
      * Avoid being verbose in declarations (private static ArrayList
        timers = null; can be static ArrayList timers -- private is the
        default, the fields are 0 inited)

Specific issues:

> +		private TimerTask GetTimerTask(Timer timer)
> +		{
> +			
> +			foreach (TimerTask timerTask in timers)
> +			{
> +				if (timerTask.timer == timer)
> +					return timerTask;
> +			}
> +			return null;
> +		}


This made me think of something that has been obvious: the Timer and
TimerTask do not need to be split. Just add Timers to the array


> +			Timer timer = this;
> +			lock(lockObj)
> +			{
> +				if (timers.Count == 1)
> +				{
> +					// i am the last timer to be disposed.
> +					if (t != null && t.IsAlive) 
> +						t = null;
> +					timers = null;
> +					initialized = false;
> +					triggeredOnce = false;
> +					
> +				}
> +				else
> +				{
> +					// then there are other timers.
> +					TimerTask timerTask = GetTimerTask(timer);
> +					timeToWaitDirty = true;
> +					timers.Remove(timerTask);								  	    Monitor.Pulse(lockObj);
> +				 }	
>  			}
> 

This code could be alot cleaner. I'd like to see it so that you don't
have to test timers.Count == 1. The way to do this would be:

      * Remove the timer from the list (like one normally would)
      * Pulse the lock

In the timer loop thread, the wait will be pulsed. Then, in this code:


> +					if (timeToWait == -1)
> +						return;// handle with some sleep..

You should set t = null. That will take the place of `initialized'.


> +		private static int GetTimeToNextEvent()
> +		{
> +			if (timers.Count == 0)
> +				return -1; // all timers are gone!
> +			if (timeToWaitDirty)
> +			{
> +				if (!triggeredOnce)
> +					time = FindMinDueTime();
> +				else
> +					time = FindMinPeriod();
> +				timeToWaitDirty = false;
> +			}
> +			return time;	
> +		}

This code is hard to understand, and somewhat non obvious. It is not
clear how DueTime works when there are multiple timers, for example.


> +		private static int FindMinDueTime()
> +		{
> +			int minDueTime = 2147483647;

No magic numbers please.


> +			if (initialized)
> +                                Add(callback,state,(int)dueTime,(int)period,this);
> +                         else
> +                                Initialize(callback,state,(int)dueTime,(int)period);

Race here (you might see that it was initialized, then the thread dies,
you add the task and it doesn't get run). I think this code should be
cleaned up a bit. I'd phrase it like:

Add (...)
if (t == null) {
	// start the thread.
}

public Timer (TimerCallback callback, object state, long dueTime, long
period)

should probably call directly to the int ctor with : this (...). That
way, you can avoid having the Init method all together. 


>  #if NET_2_0
>  		public Timer (TimerCallback callback)
>  		{
> -			Init (callback, this, Timeout.Infinite, Timeout.Infinite);
> +			if (initialized)
> +                                Add(callback,state,(int)dueTime,(int)period,this);
> +                         else
> +                                Initialize(callback,state,(int)dueTime,(int)period);
>  		}
>  #endif

This doesn't even compile on 2.0. You should use : this so that you can
put code in only one ctor.

-- Ben