3

I have a singleton service class on IIS as part of a web application (the service to be a singleton for data caching reasons). Browser clients that make requests to the service can have one of three results:

1) There is data in the cache and the data is not expired (stale) - we return this data. Very fast. 2) The cached data is expired, but another request is already querying the database. We returned the cached data. 3) The cached data is expired and no requests are making queries to the DB. This request moves forward to make the query.

However, database queries that target stored procedures with the same name have to be queued (requirement).

Thus, I wrote this queue class that is designed to queue these queries and run them consecutively, instead of concurrently. These queue classes are created as needed and stored in a list in the singleton class. When a request moves to part (3), it finds the queue class that matches its stored procedure name and submits the request to the queue class. It then waits until the data is returned from the DB so it can service the HTML request.

Unfortunately, after a few hours with this code in place, the server process maxes at 100%.

I am not sure what the best way is to go about improving it, because multi-thrread coding is not my specialty.

The queue class code looks like this:

public ReportTable GetReportTable(ReportQuery query)
{
  lock (_queue)
  {
    _queue.Enqueue(query);
    Monitor.Pulse(_queue);
  }

  lock (_queue)
  {
    var firstQueryInQueue = _queue.Peek();
    while (_inUse || firstQueryInQueue == null || firstQueryInQueue.GetHashCode() != query.GetHashCode())
    {
      Monitor.Pulse(_queue);
      Monitor.Wait(_queue);
    }

    _inUse = true;
    firstQueryInQueue = _queue.Dequeue();
    var table = firstQueryInQueue.GetNewReportTable();
    _inUse = false;

    Monitor.Pulse(_queue);
    return table;
  }
}
9
  • Starting in .NET 4.0, there's already a ConcurrentQueue written for you in the framework: ( MSDN ) Can you use that? Jan 10, 2013 at 15:33
  • I tried using that at one point Nicholas, but it didn't seem to really fit my situation. Queuing items is easy enough. It's getting the calling threads to wait without chewing up processor time that's the issue. Jan 10, 2013 at 15:47
  • Writing a concurrent queue is hard to get right, as you have found. I suggest you try to use the framework implementation if you can. If you want to learn more about using Monitor correctly, I wrote an article you might like to read : Wait and Pulse demystified Jan 10, 2013 at 15:56
  • @ChrisHolmes - OK, use BlockingCollection instead. Jan 10, 2013 at 17:35
  • I do not think a BlockingCollection solves my problem Martin. Maybe I misunderstand the documentation I've read about it. But in my particular case, the threads calling into this method are the same threads that have to wait AND consume. A single thread calling into this method has to (1) enqueue its item (2) wait until its item finds its way to the front of the queue (3) dequeue its item and do work. What I need to do is have each calling thread wait without chewing up processor time. What I'm trying to do here is take concurrent calls and make them serial without slamming the processor. Jan 10, 2013 at 18:58

2 Answers 2

0

I don't know if i understand problem but u can rewrite it very simply

private object _lockObj=new object();
public ReportTable GetReportTable(ReportQuery query)
{
  lock(_lockObj){
    var table = query.GetNewReportTable();
    return table;
  }
}
2
  • It's worth noting, about your answer: when you rely on only lock, it is my understanding that it will cause the current thread to spinwait for a while, before finally being pushed into a wait state. During that spinwait time, it's chewing cpu cycles. I need the most efficient solution possible. My very first solution was actually similar to yours, and resulting in the CPU maxing out at 100% after a few minutes with this application running. That's what I'm trying to avoid by using a slightly more complex solution. Jan 12, 2013 at 1:04
  • but you have already use lock. consider situation that two request reach your second lock then one of them lock just how my code work
    – Milad
    Jan 12, 2013 at 7:27
0

So here's what I did to fix it.

public ReportTable GetReportTable(ReportQuery query)
{
  lock (_queue)
  {
    _queue.Enqueue(query);
    Monitor.Pulse(_queue);
  }

  lock (_queue)
  {
    var firstQueryInQueue = _queue.Peek();
    while (_inUse || firstQueryInQueue == null || firstQueryInQueue.GetHashCode() != query.GetHashCode())
    {
      Monitor.Wait(_queue);
    }

    _inUse = true;
    firstQueryInQueue = _queue.Dequeue();
    var table = firstQueryInQueue.GetNewReportTable();
    _inUse = false;

    Monitor.Pulse(_queue);
    return table;
  }
}

The reason it didn't work before was because of my lack of total understanding of Monitor.Wait() and Monitor.Pulse(). I was using Pulse() in a wrong place in the code. Fortunately, there's a very good answer here that goes on to describe Wait() and Pulse() quite well.

The key is to Pulse() after the collection is changed, and give the subsequent queued threads a chance to test for the condition, which is: Is my query first in the queue? And is someone else already making a query? Failing that test, the thread calls Wait(), which puts it in the wait queue, and it doesn't tie up processor cycles. When the thread that is in the front and is performing the query completes, it flips the _inUse flag to false and calls Pulse(), waking one of the next threads so it can check the condition.

After implementing this solution and watching the Task Manager for a day, I was happy to see 1% to 5% load on the server for several hours, and the CPU never climbed to 100% as before.

I've done a lot of reading on this and it seems that PulseAll() might be the better call in this scenario, but so far Pulse() is working and we haven't encountered any issues.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.