-1

i just wrote a code, and then i found out that there are some issue with the monitor.wait, forcing me to do the operation within the locks, i wanted to now if this is a good way to keep a thread waiting,....

i'm not sure if the thread.join would do the job, as there are lots of threads running within my application, and each do specific job, that they may terminate within the time...

here is my code:

public static class TaskManager
{
    private static readonly object UpdateLock = new object();
    private static readonly object WaitLock = new object();

    private static readonly LiaisonDb _db = new LiaisonDb();
    private static List<liaQueue> _liaQueueList = new List<liaQueue>();
    private static DateTime _lastUpdate = new DateTime();

    public static liaQueue GetTask(string sessionType)
    {
        liaQueue task;
        lock (UpdateLock)
        {
            if (_lastUpdate < DateTime.Now.AddSeconds(-5))
            {
                Thread t = new Thread(UpdateCache) {IsBackground = true};
                t.Start();
                lock (WaitLock)
                {
                    Monitor.Wait(WaitLock);
                }

                _lastUpdate = DateTime.Now;
            }
            task = _liaQueueList
                .FirstOrDefault(w => w.Stat == 0
                                     && w.Type != null
                                     || string.Equals(w.Type, sessionType));
        }
        return task;
    }

    private static void UpdateCache()
    {
        try
        {
            _liaQueueList = _db.liaQueue.Where(w => w.Stat == 0).ToList();
        }
        finally
        {
            lock (WaitLock)
            {
                Monitor.Pulse(WaitLock);
            }
        }
    }
}

As you see i put two lock, and one of them is only for monitor.wait, keep the thread waiting for the answer...

i think i also have to returns null while the cache is getting refreshed?...

Hassan Faghihi
  • 1,888
  • 1
  • 37
  • 55
  • Questions seeking debugging help ("why isn't this code working?") must include the desired behavior, a specific problem or error and the shortest code necessary to reproduce it in the question itself. Questions without a clear problem statement are not useful to other readers. See: How to create a Minimal, Complete, and Verifiable example. 1 – TomTom May 25 '16 at 06:47
  • 1
    If your code IS working - you do not have a technical problem but ask for a code review. – TomTom May 25 '16 at 06:47
  • @TomTom my code is working, but the question is would this continoue working? (my first time with monitor.wait / palse), is the way i implement it the good way? and something like this. i want to know if what i did is good, and if it doesn't cause troubles, as it will become part of complex application. && how to ask for code review? – Hassan Faghihi May 25 '16 at 06:57

1 Answers1

2

From MSDN

If two threads are using Pulse and Wait to interact, this could result in a deadlock.

So, no. Your implementation would not be best practice.

It seems to me that GetTask is supposed to update the cache on a background thread, then block the calling thread until the cache was updated, and then return the first task according to a select criteria.

Since the calling thread will block (wait) for the cache to be updated, I don't quite understand the point of using a background thread in the first place. If the purpose is to prevent multiple calling threads to update the cache in parallel, use just the lock(UpdateLock) statement.

If you do want to run the cache on a background thread anyway (and wait for it), consider using the Task library instead. But I don't really se the point of it.

lock (UpdateLock)
{
  if (_lastUpdate < DateTime.Now.AddSeconds(-5)) {
    Task.Run(() => {
      _liaQueueList = _db.liaQueue.Where(w => w.Stat == 0).ToList();
    }).Wait();

    _lastUpdate = DateTime.Now;
  }
}

return _liaQueueList.FirstOrDefault(w => w.Stat == 0 && w.Type != null || string.Equals(w.Type, sessionType));
Adam B
  • 506
  • 1
  • 5
  • 13
  • Any way i look at it, and want to put your though under question, i found answer in it. BTW, using another lock i managed to stop other thread from using wait pulse together, and the other lock, is just for the sake of "synchronous block code" requirement of the Monitor.wait/pulse, since the wait pulse also release the lock, i had to use multiple lock so my first lock won't get released when i call the wait(), but the fake one. So in this case, is this acceptable for some other circumstances when there are need for multiple thread? BTW, thanks a lot. – Hassan Faghihi May 25 '16 at 07:30
  • Your question ends with "is this good practice?" (title). And the answer to that is no, because you may end up with a deadlock. Especially if you use more than one synchronization object. Thread 1 may hold lock A, but waiting for lock B. Thread 2 may hold lock B and is waiting for lock A. In summary, your solution sounds complex, and complexity adds risk. By simplifying the solution you reduce risk. – Adam B May 25 '16 at 09:12
  • You are right, but, lock 'A' surround whole stuff even lock 'B' itself, so Thread with lock 'B', either have lock 'A', or the 'B' is not locked at all and waiting for 'A' then 'B' to be achieved... Next thing, i can test it using task, but i have to remove the task latter, since some of our client run under .net 3.5 and less. server 2003, BTW i put the code directly in the body due to your right previous opinion. if someone has a thread, then with this code at least, there is no need to run another thread. – Hassan Faghihi May 25 '16 at 10:32
  • Yes, that may be true in your specific case. But as I said in my answer; I do not understand why you need the background thread. The point of a background thread is usually to add capability to do something asynchronously. Your locking mechanism and signalling seems to attempt to achieve synchronization for a async job. Why do you even need a background thread then? – Adam B May 25 '16 at 11:07
  • yes you are right, at that point i failed to see that – Hassan Faghihi May 28 '16 at 04:53