2

My goal is to write a code snippet that lets me have an exclusive access to an object (f.ex a txt file) in concurrent environment. With this in mind I was testing the simple program built on use of two System.Timers timers. Event handlers of both timers share the same lock object (Please see the code below).
The timers start simultaneously with different interval, 3s for timer1 and 1s for timer2. Timer1 supposed to work only for one cycle, during which it's event handler will sleep for 10s and thus keeping the lock.
What's surprised me is that when the lock released, I don't get all stacked in memory timer2 events (only app. every other of them). I thought, while timer1's event handler has the lock, timer2's events are stacking in memory. But that's apparently not true. Why some timer2 events dissappear?

class Program
{
    static int counter = 0;
    static readonly object locker = new object();
    System.Timers.Timer timer1;
    System.Timers.Timer timer2;

    static void Main(string[] args)
    {
        Program p = new Program();

        p.timer1 = new System.Timers.Timer(3000);
        p.timer1.Elapsed += new ElapsedEventHandler(p.Timer1EventHandler);
        p.timer1.Start();
        p.timer2 = new System.Timers.Timer(1000);
        p.timer2.Elapsed += new ElapsedEventHandler(p.Timer2EventHandler);
        p.timer2.Start();
        ThreadPool.SetMaxThreads(50, 50);
        Console.ReadLine();
    }

    void Timer1EventHandler(object sender, ElapsedEventArgs e)
    {
        timer1.Stop();
        DoThingsForTimer1Event();
    }

    void DoThingsForTimer1Event()
    {
        lock (locker)
        {
            Console.WriteLine(DateTime.Now + " Timer1 event started." + " Current thread number " + Thread.CurrentThread.ManagedThreadId);

            Thread.Sleep(10000);

            Console.WriteLine(DateTime.Now + " Timer1 event finished. Lock released.");
        }

    }

    void Timer2EventHandler(object sender, ElapsedEventArgs e)
    {
        counter++;
        lock (locker)
        {
            Console.WriteLine(DateTime.Now + " Timer2 event fired. Current thread number " + Thread.CurrentThread.ManagedThreadId +
                " Counter=" + counter);
        }                                         
    }
}

enter image description here

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
S. Neuer
  • 111
  • 7
  • 1
    `I don't get all stacked in memory timer2 events` what do you exactly mean by this, this seems to be working as you'd expect – TheGeneral Nov 07 '18 at 11:44
  • @TheGeneral As I read it, he expected the Timer2 Tick events to "stack up" in memory until the lock is released. That thing that happens when a Timer fires more often then it's Tick events can be processed. – Christopher Nov 07 '18 at 11:47
  • 1
    Though unless i am reading this wrong, this is exactly what it did – TheGeneral Nov 07 '18 at 11:48
  • 3
    If you are confused where 3-8 went - move the `++` **inside the lock**. – mjwills Nov 07 '18 at 11:52
  • 2
    _From a practical angle, you also can't use `++` from multiple threads at once safely. So if you don't put them inside the lock, you must use `Interlocked.Increment`. This isn't a major issue for you yet (since race conditions are rare), but it is still an issue._ – mjwills Nov 07 '18 at 11:55
  • @mjwills They are stacking. True. But not all. As You see there is 10s long pause during which timer2 should generate events every second, ie 10 events. At 14:36:21 we see that only 7 came out – S. Neuer Nov 07 '18 at 11:56
  • 1
    I think its just gets sick of giving you threads personally, i mean the threadpool just wont give up threads as fast as you want them most likely. i think if you change the timming to 5 second lock it will most likely work as expected – TheGeneral Nov 07 '18 at 12:06
  • https://stackoverflow.com/a/10298760/34092 and https://stackoverflow.com/a/37611613/34092 may be worth a read. – mjwills Nov 07 '18 at 12:25
  • 1
    Another way you could do this, is by giving the threads back to pool. instead of using a lock, use a `SemaphoreSlim(1,1)` between the 2 events, then awaiting the `WaitAsync`, when queues (although inefficient) will give back your threads making the scheduler happy. However, i think you should rethink the whole design, dont be `lock`ing threads like this and dont use Thread.Sleep, dont block threadpool threads – TheGeneral Nov 07 '18 at 12:27

1 Answers1

3

Thanks to @TheGeneral for identifying this as the root cause of the OP's issue.

The main issue you are running into here is that your ThreadPool is exhausted (and your Timer is using the ThreadPool), since you have a CPU with only 4 logical cores. This explains why I personally (with 12 cores) am unable to reproduce this.

As per the docs:

By default, the minimum number of threads is set to the number of processors on a system.

So the thread pool scheduler is likely starting with 4 threads. The thread pool scheduler is quite conservative. It doesn't just dish out threads as you ask for them - it sometimes delays creating them to aid in overall system performance (since spinning up threads is expensive).

To fix your immediate issue, you can prompt the thread pool to spin up more threads more quickly, using:

ThreadPool.SetMinThreads(50, 50);

This will ramp it quickly to 50, and then more conservatively after that.

Longer term though, the issue is that you are doing long running operations in the thread pool. This is a bad idea. You may wish to move them to threads, or to long running tasks (which, in practice, are threads). But both of those options have their downside. Fundamentally, you want to keep long running operations outside of the thread pool if possible.

Without understanding why you are using lock it is hard to give great advice. But one option to consider might be to use a BlockingCollection to form a queue - and then have a single separate thread processing that queue. This means your Timer events will just add an entry to the queue and then return - the brunt of the processing will be in the (single) thread that is processing the entries from the queue.

mjwills
  • 23,389
  • 6
  • 40
  • 63