[Mono-devel-list] Patch for Review - System.threading.Timerr

[Ben Maurer]

Hey,

First of all, the indentation on this file got out of whack. It's half
spaces and half tabs. Can you fix it up to use tabs (as it was before).


> +			int hashCode = this.GetHashCode();				
> +			for (int i=0;i<timers.Count;i++)
> +			{
> +				//find my timer.
> +				TimerObject timerObject = (TimerObject)timers[i];
> +				if (timerObject.uniqueid == hashCode)

Hashcodes for objects could collide (exceedingly unlikely, but technically possible).


> static void RunnerCallback(object o)
> +	{
> +			mut.WaitOne();
> +
> +			triggeredOnce = true;
> +			for (int i=0;i<timers.Count;i++)
> +			{
> +				TimerObject timerObject = (TimerObject)timers[i];
> +				int diff = (int)(DateTime.Now.Ticks - timerObject.lastSignalledTime.Ticks);
> +				if ( diff > timerObject.dueTime)
> +				{
> +					timerObject.timerCallback(timerObject.state);
> +					timerObject.lastSignalledTime = DateTime.Now;
> +				}
> +				
> +			}
> +					mut.ReleaseMutex();
> +	}

The user's callback is called within the runtime's lock. Imagine
somebody had code that did this:

Main thread:
        while (true) {
        	lock (myObj) {
        		// Create new timer
        	}
        }
        
Timer callback:
        lock (myObj) {
        	// Do something
        }
        
The user's code would deadlock.


> +		 internal struct TimerObject

I don't think this should be a struct. You are just going to end up
getting boxing. Also, we don't need value type stuff here. It should
also be renamed `TimerObject' means nothing.


> + int diff = (int)(DateTime.Now.Ticks - timerObject.lastSignalledTime.Ticks);

How does this handle spring forward / fall back (daylight's savings time
changes)? Also, what if the user changes their time zone (just got off
an airplane)?


> +		private static Mutex mut = new Mutex();
> +                private AutoResetEvent start_event  

We should avoid using mutexes and *Events. C# has more native locking in
the Monitor class. Those two classes are really interop helpers for the
io-layer. The c# stuff is cleaner -- for example, by doing lock (blah)
it does try...finally which means that stuff is handled in the case of
exceptions.


> +
> +	 public delegate void RunnerCallback(object state);
> +

You can't add public types...


>  		sealed class Runner : MarshalByRefObject
>  		{

The new design should probably get rid of this as it is just something
from the old one. Your design seems to try to put a new layer on top of
the old one, rather than just do a single threaded design from the
start.


> +				// i am the last timer to be disposed.
> +				if (t != null && t.IsAlive) 
> +				{
> +					if (t != Thread.CurrentThread)
> +						t.Abort ();
> +					t = null;

Abort is really unreliable, it can cause some nasty deadlocks. It should
really be avoided.


I think you should try rewriting the timers to do the folloing

static void TimerLoop ()
{
	while (true) {
		ArrayList ar = new ArrayList ();
		lock (lockObj) {
			int t = GetTimeToNextEvent ()
			if (t == -1)
				return; // all timers are gone
				// NOTE: make sure there are no races 
				// between this and creating another
				// event. Also may want to sleep for
				// a bit to try and get another timer
				// to avoid creating and destroying
				// thread rapidly
			
			Monitor.Wait (lockObj, t);

			foreach (Task t in alltasks)
				if (t.IsDue)
					ar.Add (t);
		}

		foreach (Task t in ar)
			t.Callback ();

		ar.Clear ();
	}
}

Then, every time you change allTasks, you .Pulse the monitor.

-- Ben