views:

512

answers:

5

This seems very noisy to me. Five lines of overhead is just too much.

m_Lock.EnterReadLock()
Try
    Return m_List.Count
Finally
    m_Lock.ExitReadLock()
End Try

So how would you simply this?

A: 

I ended up doing this, but I'm still open to better ways or flaws in my design.

Using m_Lock.ReadSection
    Return m_List.Count
End Using

This uses this extension method/class:

<Extension()> Public Function ReadSection(ByVal lock As ReaderWriterLockSlim) As ReadWrapper
 Return New ReadWrapper(lock)
End Function


Public NotInheritable Class ReadWrapper
 Implements IDisposable

 Private m_Lock As ReaderWriterLockSlim
 Public Sub New(ByVal lock As ReaderWriterLockSlim)
  m_Lock = lock
  m_Lock.EnterReadLock()
 End Sub
 Public Sub Dispose() Implements IDisposable.Dispose
  m_Lock.ExitReadLock()
 End Sub

End Class
Jonathan Allen
Two thoughts: first, you should clear m_Lock so that a double Dispose() doesn't cause issues (unlikely, but...)second - there is no need for the caller to know about ReadWrapper if IDisposable would suffice. But I like it ;-p
Marc Gravell
Good point, I didn't want to expose the ReadWrapper type anyways.
Jonathan Allen
+2  A: 

I was thinking the same, but in C# ;-p

using System;
using System.Threading;

class Program
{
    static void Main()
    {
        ReaderWriterLockSlim sync = new ReaderWriterLockSlim();

        using (sync.Read())
        {
           // etc    
        }
    }


}
public static class ReaderWriterExt
{
    sealed class ReadLockToken : IDisposable
    {
        private ReaderWriterLockSlim sync;
        public ReadLockToken(ReaderWriterLockSlim sync)
        {
            this.sync = sync;
            sync.EnterReadLock();
        }
        public void Dispose()
        {
            if (sync != null)
            {
                sync.ExitReadLock();
                sync = null;
            }
        }
    }
    public static IDisposable Read(this ReaderWriterLockSlim obj)
    {
        return new ReadLockToken(obj);
    }
}
Marc Gravell
A: 
Emperor XLII
A: 

This is not my invention but it certainly has made by hair a little less gray.

internal static class ReaderWriteLockExtensions
{
    private struct Disposable : IDisposable
    {
        private readonly Action m_action;
        private Sentinel m_sentinel;

        public Disposable(Action action)
        {
            m_action = action;
            m_sentinel = new Sentinel();
        }

        public void Dispose()
        {
            m_action();
            GC.SuppressFinalize(m_sentinel);
        }
    }

    private class Sentinel
    {
        ~Sentinel()
        {
            throw new InvalidOperationException("Lock not properly disposed.");
        }
    }

    public static IDisposable AcquireReadLock(this ReaderWriterLockSlim lock)
    {
        lock.EnterReadLock();
        return new Disposable(lock.ExitReadLock);
    }

    public static IDisposable AcquireUpgradableReadLock(this ReaderWriterLockSlim lock)
    {
        lock.EnterUpgradeableReadLock();
        return new Disposable(lock.ExitUpgradeableReadLock);
    }

    public static IDisposable AcquireWriteLock(this ReaderWriterLockSlim lock)
    {
        lock.EnterWriteLock();
        return new Disposable(lock.ExitWriteLock);
    }
}

How to use:

using (m_lock.AcquireReadLock())
{
    // Do stuff
}
Patrik
A: 

All the solutions posted so far are at risk of deadlock. A using block like this:

ReaderWriterLockSlim sync = new ReaderWriterLockSlim();
using (sync.Read())
{
  // Do stuff
}

gets converted into something like this:

ReaderWriterLockSlim sync = new ReaderWriterLockSlim();
IDisposable d = sync.Read();
try
{
  // Do stuff
}
finally
{
  d.Dispose();
}

This means that a ThreadAbortException (or similar) could happen between sync.Read() and the try block. When this happens the finally block never gets called, and the lock is never released!

For more information, and a better implementation see: Deadlock with ReaderWriterLockSlim and other lock objects

Also, from Joe Duffy's Blog

Next, the lock is not robust to asynchronous exceptions such as thread aborts and out of memory conditions. If one of these occurs while in the middle of one of the lock’s methods, the lock state can be corrupt, causing subsequent deadlocks, unhandled exceptions, and (sadly) due to the use of spin locks internally, a pegged 100% CPU. So if you’re going to be running your code in an environment that regularly uses thread aborts or attempts to survive hard OOMs, you’re not going to be happy with this lock.

Eric Lathrop
If someone is throwing around ThreadAbortExceptions then there are far more serious problems than just a deadlock. Then ONLY time a ThreadAbortException is appropriate is when it is raised by the thread itself such as when calling HttpResponse.End.
Jonathan Allen