CODEWITHSUNDEEP
c#lockingfactory-pattern
Joe
Joe

Reputation: 42646

Locking in a factory method

I am interfacing with a back-end system, where I must never ever have more than one open connection to a given object (identified by it's numeric ID), but different consumers may be opening and closing them independently of one another.

Roughly, I have a factory class fragment like this:

private Dictionary<ulong, IFoo> _openItems = new Dictionary<ulong, IFoo>();
private object _locker = new object();

public IFoo Open(ulong id)
{
    lock (_locker)
    {
        if (!_openItems.ContainsKey(id))
        {
            _openItems[id] = _nativeResource.Open(id);
        }

        _openItems[id].RefCount++;

        return _openItems[id];
    }
}

public void Close(ulong id)
{
    lock (_locker)
    {
        if (_openItems.ContainsKey(id))
        {
            _openItems[id].RefCount--;
            if (_openItems[id].RefCount == 0)
            {
                _nativeResource.Close(id);
                _openItems.Remove(id);
            }
        }
    }
}

Now, here is the problem. In my case, _nativeResource.Open is very slow. The locking in here is rather naive and can be very slow when there are a lot of different concurrent .Open calls, even though they are (most likely) referring to different ids and don't overlap, especially if they are not in the _openItems cache.

How do I structure the locking so that I am only preventing concurrent access to a specific ID and not to all callers?

Upvotes: 0

Views: 437

Answers (2)

Trevor Pilley
Trevor Pilley

Reputation: 16413

If you are on .net 4, you could try the ConcurrentDictionary with something along these lines:

private ConcurrentDictionary<ulong, IFoo> openItems = new ConcurrentDictionary<ulong, IFoo>();
private object locker = new object();

public IFoo Open(ulong id)
{
    var foo = this.openItems.GetOrAdd(id, x => nativeResource.Open(x));

    lock (this.locker)
    {
        foo.RefCount++;
    }

    return foo;
}

public void Close(ulong id)
{
    IFoo foo = null;

    if (this.openItems.TryGetValue(id, out foo))
    {
        lock (this.locker)
        {
            foo.RefCount--;

            if (foo.RefCount == 0)
            {
                if (this.openItems.TryRemove(id, out foo))
                {
                    this.nativeResource.Close(id);
                }
            }
        }
    }
}

If anyone can see any glaring issues with that, please let me know!

Upvotes: 1

Chris Shain
Chris Shain

Reputation: 51359

What you may want to look into is a striped locking strategy. The idea is that you share N locks for M items (possible ID's in your case), and choose a lock such that for any ID the lock chosen is always the same one. The classic way of choosing locks for this technique is modulo division- simply divide M by N, take the remainder, and use the lock with that index:

// Assuming the allLocks class member is defined as follows:
private static AutoResetEvent[] allLocks = new AutoResetEvent[10];


// And initialized thus (in a static constructor):
for (int i = 0; i < 10; i++) {
    allLocks[i] = new AutoResetEvent(true);
}


// Your method becomes
var lockIndex = id % allLocks.Length;
var lockToUse = allLocks[lockIndex];

// Wait for the lock to become free
lockToUse.WaitOne();
try {
    // At this point we have taken the lock

    // Do the work
} finally {
    lockToUse.Set();
}

Upvotes: 3

Related Questions