92

I've got a bunch of properties which I am going to use read/write locks on. I can implement them either with a try finally or a using clause.

In the try finally I would acquire the lock before the try, and release in the finally. In the using clause, I would create a class which acquires the lock in its constructor, and releases in its Dispose method.

I'm using read/write locks in a lot of places, so I've been looking for ways that might be more concise than try finally. I'm interested in hearing some ideas on why one way may not be recommended, or why one might be better than another.

Method 1 (try finally):

static ReaderWriterLock rwlMyLock_m  = new ReaderWriterLock();
private DateTime dtMyDateTime_m
public DateTime MyDateTime
{
    get
    {
        rwlMyLock_m .AcquireReaderLock(0);
        try
        {
            return dtMyDateTime_m
        }
        finally
        {
            rwlMyLock_m .ReleaseReaderLock();
        }
    }
    set
    {
        rwlMyLock_m .AcquireWriterLock(0);
        try
        {
            dtMyDateTime_m = value;
        }
        finally
        {
            rwlMyLock_m .ReleaseWriterLock();
        }
    }
}

Method 2:

static ReaderWriterLock rwlMyLock_m  = new ReaderWriterLock();
private DateTime dtMyDateTime_m
public DateTime MyDateTime
{
    get
    {
        using (new ReadLock(rwlMyLock_m))
        {
            return dtMyDateTime_m;
        }
    }
    set
    {
        using (new WriteLock(rwlMyLock_m))
        {
            dtMyDateTime_m = value;
        }
    }
}

public class ReadLock : IDisposable
{
    private ReaderWriterLock rwl;
    public ReadLock(ReaderWriterLock rwl)
    {
        this.rwl = rwl;
        rwl.AcquireReaderLock(0);
    }

    public void Dispose()
    {
        rwl.ReleaseReaderLock();
    }
}

public class WriteLock : IDisposable
{
    private ReaderWriterLock rwl;
    public WriteLock(ReaderWriterLock rwl)
    {
        this.rwl = rwl;
        rwl.AcquireWriterLock(0);
    }

    public void Dispose()
    {
        rwl.ReleaseWriterLock();
    }
}
1
  • 1
    Like already stated in many answers, Method 2 is very good, but to avoid garbage on the heap each time you use the lock, you should change ReadLock and WriteLock to structs. Even though the using statement uses the IDisposable interface of a struct, C# is clever enough to avoid boxing! Mar 17, 2020 at 8:44

15 Answers 15

107

From MSDN, using Statement (C# Reference)

The using statement ensures that Dispose is called even if an exception occurs while you are calling methods on the object. You can achieve the same result by putting the object inside a try block and then calling Dispose in a finally block; in fact, this is how the using statement is translated by the compiler. The code example earlier expands to the following code at compile time (note the extra curly braces to create the limited scope for the object):

{
  Font font1 = new Font("Arial", 10.0f);
  try
  {
    byte charset = font1.GdiCharSet;
  }
  finally
  {
    if (font1 != null)
      ((IDisposable)font1).Dispose();
  }
}

So basically, it is the same code but with a nice automatic null-checks and an extra scope for your variable. The documentation also states that it "ensures the correct use of IDisposable object" so you might as well gets even better framework support for any obscure cases in the future.

So go with option 2.

Having the variable inside a scope that ends immediately after it's no longer needed is also a plus.

6
  • what is best to release a resource which can not be instantiated within using statement or being reused or being passed as out param?try/catch!!!
    – bjan
    May 11, 2012 at 5:20
  • 2
    @bjan well, why are you even considering using in that case? that's not what using is for.
    – chakrit
    May 11, 2012 at 6:40
  • 1
    that is why i also mention try/catch as it looks the only way to handle via try/catch/finally block. hoped using could handle this too
    – bjan
    May 11, 2012 at 6:57
  • Well, yeah try/finally's your only option in that case I guess. IMO, though, I think there should always be some object/piece of code responsible for maintaining lifetime of the object in such cases (where Dispose() should always be called.) If one class only handles the instantiation and somebody else has to remember to dispose it I think it kinds of smells a bit there. Not sure how one would add that at the language level.
    – chakrit
    May 11, 2012 at 7:23
  • I think the only reason to avoid "using" is to avoid the disposable completely since it's yet another object instantiation I guess? Jul 31, 2017 at 12:46
12

I definitely prefer the second method. It is more concise at the point of usage, and less error prone.

In the first case someone editing the code has to be careful not to insert anything between the Acquire(Read|Write)Lock call and the try.

(Using a read/write lock on individual properties like this is usually overkill though. They are best applied at a much higher level. A simple lock will often suffice here since the possibility of contention is presumably very small given the time the lock is held for, and acquiring a read/write lock is a more expensive operation than a simple lock).

5
  • How about being fail-safe? I know a try finally will always fire the finally block, is there any way the dispose won't get called?
    – Jeremy
    Nov 10, 2008 at 19:38
  • 1
    No, the using statement is essentially syntactic sugar for the try/finally pattern. Nov 10, 2008 at 19:42
  • The using model ensures that the object will always be disposed. Nov 10, 2008 at 19:42
  • If you use reflector you will see that the using block is translated by the compiler into a try ... finally construct, so at the IL level they are equivalent. The 'using' is just syntatic sugar.
    – Rob Walker
    Nov 10, 2008 at 19:42
  • ^^ remember there are potential scenarios where finally is not called. A power cut for example. ;) Nov 10, 2008 at 19:44
11

Consider the possibility that both solutions are bad because they mask exceptions.

A try without a catch should obviously be a bad idea; see MSDN for why the using statement is likewise dangerous.

Note also Microsoft now recommends ReaderWriterLockSlim instead of ReaderWriterLock.

Finally, note that the Microsoft examples use two try-catch blocks to avoid these issues, e.g.

try
{
    try
    {
         //Reader-writer lock stuff
    }
    finally
    {
         //Release lock
    }
 }
 catch(Exception ex)
 {
    //Do something with exception
 }

A simple, consistent, clean solution is a good goal, but assuming you can't just use lock(this){return mydateetc;}, you might reconsider the approach; with more info I'm sure Stack Overflow can help ;-)

3
  • 3
    A try finally does not necessarily mask the exception. In my example, the lock is aquired, then if any exception is thrown within the scope, the lock will be freed, but the exception will still bubble up.
    – Jeremy
    Nov 10, 2008 at 21:22
  • 1
    @Jeremy: if your finally block throws an exception it will mask the exception thrown in your try block - that's what the msdn article was saying was the (same) problem with the using syntax Nov 11, 2008 at 5:14
  • 4
    It won't mask the exception, it'll replace the exception in exactly the same way as yours will.
    – Jon Hanna
    Mar 26, 2015 at 16:05
5

I personally use the C# "using" statement as often as possible, but there are a few specific things that I do along with it to avoid the potential issues mentioned. To illustrate:

void doSomething()
{
    using (CustomResource aResource = new CustomResource())
    {
        using (CustomThingy aThingy = new CustomThingy(aResource))
        {
            doSomething(aThingy);
        }
    }
}

void doSomething(CustomThingy theThingy)
{
    try
    {
        // play with theThingy, which might result in exceptions
    }
    catch (SomeException aException)
    {
        // resolve aException somehow
    }
}

Note that I separate the "using" statement into one method and the use of the object(s) into another method with a "try"/"catch" block. I may nest several "using" statements like this for related objects (I sometimes go three or four deep in my production code).

In my Dispose() methods for these custom IDisposable classes, I catch exceptions (but NOT errors) and log them (using Log4net). I have never encountered a situation where any of those exceptions could possibly affect my processing. The potential errors, as usual, are allowed to propagate up the call stack and typically terminate processing with an appropriate message (the error and stack trace) logged.

If I somehow encountered a situation where a significant exception could occur during Dispose(), I would redesign for that situation. Frankly, I doubt that will ever happen.

Meanwhile, the scope and cleanup advantages of "using" make it one of my most favorite C# features. By the way, I work in Java, C#, and Python as my primary languages, with lots of others thrown in here and there, and "using" is one of my most favorite language features all around because it is a practical, everyday workhorse.

1
  • Tip, for code readability: Align and compress the uses statements by not having curly braces except for the inner "using". Feb 7, 2017 at 7:17
4

I like the 3rd option

private object _myDateTimeLock = new object();
private DateTime _myDateTime;

public DateTime MyDateTime{
  get{
    lock(_myDateTimeLock){return _myDateTime;}
  }
  set{
    lock(_myDateTimeLock){_myDateTime = value;}
  }
}

Of your two options, the second option is the cleanest and easier to understand what's going on.

3
  • lock statement does not work the same way ReaderWriterLock does.
    – chakrit
    Nov 10, 2008 at 19:53
  • @chakrit: No, but unless you know there actually is lock contention they're likely to be more performant. Nov 10, 2008 at 20:01
  • True, but you should always go for the smiple lock first. The subby's question doesn't state anything about performance issues or requirements that won't allow a lock. Therefore there's no reason to try and be slick. Just lock it and rock it.
    – user1228
    Nov 10, 2008 at 20:03
4

"Bunch of properties" and locking at the property getter and setter level looks wrong. Your locking is much too fine-grained. In most typical object usage, you'd want to make sure that you acquired a lock to access more than one property at the same time. Your specific case might be different but I kinda doubt it.

Anyway, acquiring the lock when you access the object instead of the property will significantly cut down on the amount of locking code you'll have to write.

1
  • Yes, I defintely see your point. Most of my properties are actually bools, ints, which I woudln't need to lock because they should be atomic. There are some datetime, string ones that I'd want the lock on. because the minority would need locks, I thight best to put it on the property
    – Jeremy
    Nov 10, 2008 at 22:34
4

DRY says: second solution. The first solution duplicates the logic of using a lock, whereas the second does not.

2

Try/Catch blocks are generally for exception handling, while using blocks are used to ensure that the object is disposed.

For the read/write lock a try/catch might be the most useful, but you could also use both, like so:

using (obj)
{
  try { }
  catch { }
}

so that you can implicitly call your IDisposable interface as well as make exception handling concise.

1

The following creates extension methods for the ReaderWriterLockSlim class that allow you to do the following:

var rwlock = new ReaderWriterLockSlim();
using (var l = rwlock.ReadLock())
{
     // read data
}
using (var l = rwlock.WriteLock())
{
    // write data
}

Here's the code:

static class ReaderWriterLockExtensions() {
    /// <summary>
    /// Allows you to enter and exit a read lock with a using statement
    /// </summary>
    /// <param name="readerWriterLockSlim">The lock</param>
    /// <returns>A new object that will ExitReadLock on dispose</returns>
    public static OnDispose ReadLock(this ReaderWriterLockSlim readerWriterLockSlim)
    {
        // Enter the read lock
        readerWriterLockSlim.EnterReadLock();
        // Setup the ExitReadLock to be called at the end of the using block
        return new OnDispose(() => readerWriterLockSlim.ExitReadLock());
    }
    /// <summary>
    /// Allows you to enter and exit a write lock with a using statement
    /// </summary>
    /// <param name="readerWriterLockSlim">The lock</param>
    /// <returns>A new object that will ExitWriteLock on dispose</returns>
    public static OnDispose WriteLock(this ReaderWriterLockSlim rwlock)
    {
        // Enter the write lock
        rwlock.EnterWriteLock();
        // Setup the ExitWriteLock to be called at the end of the using block
        return new OnDispose(() => rwlock.ExitWriteLock());
    }
}

/// <summary>
/// Calls the finished action on dispose.  For use with a using statement.
/// </summary>
public class OnDispose : IDisposable
{
    Action _finished;

    public OnDispose(Action finished) 
    {
        _finished = finished;
    }

    public void Dispose()
    {
        _finished();
    }
}
0

I think method 2 would be better.

  • Simpler and more readable code in your properties.
  • Less error-prone since the locking code doesn't have to be re-written several times.
0

While I agree with many of the above comments, including the granularity of the lock and questionable exception handling, the question is one of approach. Let me give you one big reason why I prefer using over the try {} finally model... abstraction.

I have a model very similar to yours with one exception. I defined a base interface ILock and in it I provided one method called Acquire(). The Acquire() method returned the IDisposable object and as a result means that as long as the object I am dealing with is of type ILock that it can be used to do a locking scope. Why is this important?

We deal with many different locking mechanisms and behaviors. Your lock object may have a specific timeout that employs. Your lock implementation may be a monitor lock, reader lock, writer lock or spin lock. However, from the perspective of the caller all of that is irrelevant, what they care about is that the contract to lock the resource is honored and that the lock does it in a manner consistent with it's implementation.

interface ILock {
    IDisposable Acquire();
}

class MonitorLock : ILock {
    IDisposable Acquire() { ... acquire the lock for real ... }
}

I like your model, but I'd consider hiding the lock mechanics from the caller. FWIW, I've measured the overhead of the using technique versus the try-finally and the overhead of allocating the disposable object will have between a 2-3% performance overhead.

0

I'm surprised no one has suggested encapsulating the try-finally in anonymous functions. Just like the technique of instantiating and disposing of classes with the using statement, this keeps the locking in one place. I prefer this myself only because I'd rather read the word "finally" than the word "Dispose" when I'm thinking about releasing a lock.

class StackOTest
{
    private delegate DateTime ReadLockMethod();
    private delegate void WriteLockMethod();

    static ReaderWriterLock rwlMyLock_m  = new ReaderWriterLock();
    private DateTime dtMyDateTime_m;
    public DateTime MyDateTime
    {
        get
        {
            return ReadLockedMethod(
                rwlMyLock_m,
                delegate () { return dtMyDateTime_m; }
            );
        }
        set
        {
            WriteLockedMethod(
                rwlMyLock_m,
                delegate () { dtMyDateTime_m = value; }
            );
        }
    }

    private static DateTime ReadLockedMethod(
        ReaderWriterLock rwl,
        ReadLockMethod method
    )
    {
        rwl.AcquireReaderLock(0);
        try
        {
            return method();
        }
        finally
        {
            rwl.ReleaseReaderLock();
        }
    }

    private static void WriteLockedMethod(
        ReaderWriterLock rwl,
        WriteLockMethod method
    )
    {
        rwl.AcquireWriterLock(0);
        try
        {
            method();
        }
        finally
        {
            rwl.ReleaseWriterLock();
        }
    }
}
0

SoftwareJedi, I don't have an account, so I can't edit my answers.

In any case, the previous version wasn't really good for general purpose use since the read lock always required a return value. This fixes that:

class StackOTest
{
    static ReaderWriterLock rwlMyLock_m  = new ReaderWriterLock();
    private DateTime dtMyDateTime_m;
    public DateTime MyDateTime
    {
        get
        {
            DateTime retval = default(DateTime);
            ReadLockedMethod(
                delegate () { retval = dtMyDateTime_m; }
            );
            return retval;
        }
        set
        {
            WriteLockedMethod(
                delegate () { dtMyDateTime_m = value; }
            );
        }
    }

    private void ReadLockedMethod(Action method)
    {
        rwlMyLock_m.AcquireReaderLock(0);
        try
        {
            method();
        }
        finally
        {
            rwlMyLock_m.ReleaseReaderLock();
        }
    }

    private void WriteLockedMethod(Action method)
    {
        rwlMyLock_m.AcquireWriterLock(0);
        try
        {
            method();
        }
        finally
        {
            rwlMyLock_m.ReleaseWriterLock();
        }
    }
}
0

Actually in your first example, to make the solutions comparable, you would also implement IDisposable there as well. Then you'd call Dispose() from the finally block instead of releasing the lock directly.

Then you'd be "apples to apples" implementation (and MSIL)-wise (MSIL will be the same for both solutions). It's still probably a good idea to use using because of the added scoping and because the Framework will ensure proper usage of IDisposable (the latter being less beneficial if you're implementing IDisposable yourself).

-1

Silly me. There's a way to make that even simpler by making the locked methods part of each instance (instead of static like in my previous post). Now I really prefer this because there's no need to pass `rwlMyLock_m' off to some other class or method.

class StackOTest
{
    private delegate DateTime ReadLockMethod();
    private delegate void WriteLockMethod();

    static ReaderWriterLock rwlMyLock_m  = new ReaderWriterLock();
    private DateTime dtMyDateTime_m;
    public DateTime MyDateTime
    {
        get
        {
            return ReadLockedMethod(
                delegate () { return dtMyDateTime_m; }
            );
        }
        set
        {
            WriteLockedMethod(
                delegate () { dtMyDateTime_m = value; }
            );
        }
    }

    private DateTime ReadLockedMethod(ReadLockMethod method)
    {
        rwlMyLock_m.AcquireReaderLock(0);
        try
        {
            return method();
        }
        finally
        {
            rwlMyLock_m.ReleaseReaderLock();
        }
    }

    private void WriteLockedMethod(WriteLockMethod method)
    {
        rwlMyLock_m.AcquireWriterLock(0);
        try
        {
            method();
        }
        finally
        {
            rwlMyLock_m.ReleaseWriterLock();
        }
    }
}
0

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.