26

In this article: http://msdn.microsoft.com/en-us/magazine/cc164015.aspx the author states that System.Threading.Timer is not thread-safe.

Since then this has been repeated on blogs, in Richter's book "CLR via C#", on SO, but this is never justified.

Moreover the MSDN documentation assures "This type is thread safe."

1) Who tells the truth?

2) If this is the original article what makes System.Threading.Timer not thread-safe and how its wrapper System.Timers.Timer achieves more thread-safety?

Thanks

Pragmateek
  • 13,174
  • 9
  • 74
  • 108
  • Thread-safety for what operations? Do you want to change the timer concurrently? – usr Oct 24 '13 at 21:48
  • 7
    Thread-safety can mean many things so everybody is right. – H H Oct 24 '13 at 21:49
  • @usr: well I'm maybe too basic but for me a type is said to be thread-safe if any instance can be used from 654139846535821 threads concurrently without corrupting its state. If you have any other definition please share. Thanks :) – Pragmateek Oct 24 '13 at 21:52
  • @HenkHolterman: OK I've defined what is thread-safety for me in my comment. Thanks :) – Pragmateek Oct 24 '13 at 21:53
  • 7
    That many threads will seriously corrupt the state of your PC. – H H Oct 24 '13 at 21:54
  • 3
    The problem is that neither page is very clear about what it means. – H H Oct 24 '13 at 21:54
  • @HenkHolterman: my computer is Deep Thought so no problem. ;) As for the definition of thread-safety I thought there was an universally accepted one. Mine is equivalent to the definition of Wikipedia which states: "A piece of code is thread-safe if it only manipulates shared data structures in a manner that guarantees safe execution by multiple threads at the same time.". – Pragmateek Oct 24 '13 at 21:59
  • 5
    @Pragmateek: Read that definition carefully; it says "a thread safe program is a program that is safe for threads"! It would be hard to come up with a *less* useful definition. – Eric Lippert Oct 24 '13 at 23:15
  • @EricLippert: aaaahhhh an (ex-)insider! ;) So Eric what's your opinion on this "issue"? :) – Pragmateek Oct 24 '13 at 23:27
  • 1
    What I was saying is that you hardly can use the timer concurrently in a useful way, even if its members were thread-safe. What would it mean to concurrently set the `Enabled` property to two different values simultaneously? One one write would come through. That's why I request that you say what you want to do. – usr Oct 25 '13 at 07:51
  • 2
    @usr: I have no real use-case, it's just out of curiosity. Moreover I've learnt a golden rule in programming: never assume something is a non-sense, because you'll almost always find somebody with a relevant use-case. ;) – Pragmateek Oct 25 '13 at 09:34

2 Answers2

47

No, that's not the way it works. The .NET asynchronous Timer classes are perfectly thread-safe. The problem with thread-safety is that it is not a transitive property, it doesn't make the other code that's executed thread-safe as well. The code that you wrote, not a .NET Framework programmer.

It is the same kind of problem with the very common assumption that Windows UI code is fundamentally thread-unsafe. It is not, the code inside Windows is perfectly thread-safe. The problem is all the code that runs that is not part of Windows and not written by a Microsoft programmer. There's always a lot of that code, triggered by a SendMessage() call. Which runs custom code that a programmer wrote. Or code he didn't write, like a hook installed by some utility. Code that assumes that the program doesn't make it difficult and just executes message handlers on one thread. He usually does, not doing that buys him a lot of trouble.

Same problem with the System.Timers.Timer.Elapsed event and the System.Threading.Timer callback. Programmers make lots of mistakes writing that code. It runs complete asynchronously on an arbitrary threadpool thread, touching any shared variable really does require locking to protect state. Very easy to overlook. And worse, much worse, very easy to get yourself into a pile of trouble when the code runs again, before the previous invocation stopped running. Triggered when the timer interval is too low or the machine is too heavily loaded. Now there are two threads running the same code, that rarely comes to a good end.

Threading is hard, news at eleven.

Hans Passant
  • 922,412
  • 146
  • 1,693
  • 2,536
  • 2
    Thanks Hans for reminding us this fundamental principle. I'm aware of this issue (learnt it the hard way ;)). But some peoples (and not just anyone) states that a fundamental difference between these timers is thread-safety. As discussed with Henk and as you say either both should be considered thread-safe or both should be considered not thread-safe. Maybe the explanation is simple: the original source was wrong (e.g. because based on an early alpha implementation) and the others simply repeated this wrong statement. But I'd like to be sure. – Pragmateek Oct 24 '13 at 23:01
  • 1
    This is an excellent answer. @Pragmateek, you should accept this as the answer IMHO – Sudhanshu Mishra Nov 14 '14 at 09:14
  • 4
    @mishrsud: the reason why this answer was not accepted is not its quality which is indeed excellent but the reasonable doubt that still exists because other sources had opposite views on the topic. But let's consider Hans as the definitive source (IMHO a reasonable hypothesis), so I'll accept it. :) – Pragmateek Nov 14 '14 at 10:53
  • 2
    Just as a warning: The property Enabled in System.Timers.Timer is NOT Thread save. Just look at the source of it. (Like for almost all .Net classes the doku tells us: Any public static members of this type are thread safe. Any instance members are not guaranteed to be thread safe.) – Markus Sep 13 '16 at 12:22
  • 2
    _"The .NET asynchronous Timer classes are perfectly thread-safe"_ -- no, only the `System.Threading.Timer` class is documented to be thread-safe. The `System.Timers.Timer` class is specifically documented as _not_ being guaranteed to be thread-safe, and it would be unwise to assume otherwise. – Peter Duniho Nov 05 '17 at 19:59
8

The System.Timers.Timer class is not thread safe. Here is how it can be proved. A single Timer instance is created, and its property Enabled is toggled endlessly by two different threads that are running in parallel. If the class is thread safe, its internal state will not be corrupted. Lets see...

var timer = new System.Timers.Timer();
var tasks = Enumerable.Range(1, 2).Select(x => Task.Run(() =>
{
    while (true)
    {
        timer.Enabled = true;
        timer.Enabled = false;
    }
})).ToArray();
Task.WhenAny(tasks).Unwrap().GetAwaiter().GetResult();

This program is not running for too long. An exception is thrown almost immediately. It is either a NullReferenceException or an ObjectDisposedException:

System.NullReferenceException: Object reference not set to an instance of an object.
   at System.Timers.Timer.UpdateTimer()
   at System.Timers.Timer.set_Enabled(Boolean value)
   at Program.<>c__DisplayClass1_0.<Main>b__1()
   at System.Threading.Tasks.Task`1.InnerInvoke()
   at System.Threading.Tasks.Task.<>c.<.cctor>b__274_0(Object obj)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of stack trace from previous location where exception was thrown ---
   at Program.Main(String[] args)
Press any key to continue . . .

System.ObjectDisposedException: Cannot access a disposed object.
   at System.Threading.TimerQueueTimer.Change(UInt32 dueTime, UInt32 period)
   at System.Threading.Timer.Change(Int32 dueTime, Int32 period)
   at System.Timers.Timer.UpdateTimer()
   at System.Timers.Timer.set_Enabled(Boolean value)
   at Program.<>c__DisplayClass1_0.<Main>b__1()
   at System.Threading.Tasks.Task`1.InnerInvoke()
   at System.Threading.Tasks.Task.<>c.<.cctor>b__274_0(Object obj)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
--- End of stack trace from previous location where exception was thrown ---
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)
   at System.Threading.Tasks.Task.ExecuteWithThreadLocal(Task& currentTaskSlot, Thread threadPoolThread)
--- End of stack trace from previous location where exception was thrown ---
   at Program.Main(String[] args)
Press any key to continue . . .

The reason this happens is quite evident, after studying the source code of the class. There is no synchronization when the internal fields of the class are changed. So synchronizing manually the access to a Timer instance is mandatory, when this instance is mutated by multiple threads in parallel. For example the program below runs forever without throwing any exception.

var locker = new object();
var timer = new System.Timers.Timer();
var tasks = Enumerable.Range(1, 2).Select(x => Task.Run(() =>
{
    while (true)
    {
        lock (locker) timer.Enabled = true;
        lock (locker) timer.Enabled = false;
    }
})).ToArray();
Task.WhenAny(tasks).Unwrap().GetAwaiter().GetResult();

Regarding the System.Threading.Timer class, it has no properties, and its single method Change can be called by multiple threads in parallel without any exceptions thrown. Its source code indicates that it's thread safe, since a lock is used internally.

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • 1
    Thanks for this really interesting test. That's becoming ridiculous as the one that should be thread-safe is not, and the other that should not be is (I've tried to break it but impossible). :) – Pragmateek Mar 29 '20 at 14:32
  • 2
    This is the right answer and is also consistent with the official documentation of both classes. – Mo B. Jun 01 '22 at 13:22