Problem with using Semaphore to protect queue

Question

I am using following code to limit use of resources.

Once in a while(after 3-4 days of successful run) I get queue empty exception or the returned object is found to be null.

I am wondering if I am limiting only 5 threads to enter this Get method, how come this happens.

The places where GetConnection is called, ReleaseConnection is also definitely called within the Finally block.

With each call, I am also logging no. of resources in the queue. The queue count never seems to be going more than 5.

Semaphore smphSync = new Semaphore(0, 5);

Queue<IResource> resources;

private IResource GetResource()

{

    smphSync.WaitOne();

    IResource res = resources.Dequeue();

    return res;
}

private ReleaseResource(IResource res)

{

    resources.Enqueue(res);

    smphSync.Release();
}

My question is, Do I need to synchronize the access to queue (resources instance), using lock/Monitor?

Answer 1

I added lock() around my ThreadSafeQueue class and recently added a TryDequeue() method. More details in this post. Definitely improved multiple thread collisions I was frequently seeing before (most notably returning a null object when no nulls existed in the Queue).

Edit: Checked in the TryDequeue() method and updated the link to the correct changeset.

Answer 2

None of the standard .NET collections are thread-safe by default. They *cannot*be accessed concurrently without some kind of memory barrier preventing concurrent access.

In your case, the semaphore prevents more than five threads from accessing resources but nothing prevents any of those five concurrent threads from entering Dequeue() or Enqueue() at the same time. It is entirely possible that a rare race condition occurs amongst those threads which results in the corruption of the queue. You should really put a lock around the resources queue itself.

I would also advise you perform a test inside the lock to make sure that the queue still has items to remove, before you attempt to call Dequeue(). However, since I don't know the specifics of how your code works, I leave it to you to decide if that's relevant.

Semaphore smphSync = new Semaphore(0, 5);
Queue<IResource> resources;
private _lockObj = new object();

private IResource GetResource()
{
    smphSync.WaitOne();
    lock( _lockObj ) 
    {
        IResource res = resources.Dequeue();
        return res;
    }
}

private ReleaseResource(IResource res)
{
    lock( _lockObj )
    {
        resources.Enqueue(res);
    }
    smphSync.Release();
}