Is this a dangerous locking pattern?

Question

I have an enumerator written in C#, which looks something like this:

try
{
    ReadWriteLock.EnterReadLock();
    yield return foo;
    yield return bar;
    yield return bash;
}
finally
{
    if (ReadWriteLock.IsReadLockHeld)
        ReadWriteLock.ExitReadLock();
}

I believe this may be a dangerous locking pattern, as the ReadWriteLock will only be released if the enumeration is complete, otherwise the lock is left hanging and is never released, am I correct? If so, what's the best way to combat this?

Answer 1

No, the finally block will always be executed, pretty much unless somebody pulls the plug from the computer (well and a few other exceptions).

public static IEnumerable<int> GetNumbers() {
    try
    {
        Console.WriteLine("Start");
        yield return 1;
        yield return 2;
        yield return 3;
    }
    finally
    {
        Console.WriteLine("Finish");
    }
}

...

foreach(int i in GetNumbers()) {
    Console.WriteLine(i);
    if(i == 2) break;
}

The output of the above will be

Start
1
2
Finish

Note that in C# you write yield return, not just yield. But I guess that was just a typo.

Answer 2

Finally will be executed in any way, but for locking in may not be safe. Compare following methods:

class Program
{
    static IEnumerable<int> meth1()
    {
        try
        {
            Console.WriteLine("Enter");
            yield return 1;
            yield return 2;
            yield return 3;
        }
        finally
        {
            Console.WriteLine("Exit");
        }
    }

    static IEnumerable<int> meth2()
    {
        try
        {
            Console.WriteLine("Enter");
            return new int[] { 1, 2, 3 };
        }
        finally
        {
            Console.WriteLine("Exit");
        }
    }

    static public void Main()
    {
        foreach (int i in meth1())
        {
            Console.WriteLine("In");
        }
        Console.WriteLine();
        foreach (int i in meth2())
        {
            Console.WriteLine("In");
        }   
    }
}

Output is:

Enter
In
In
In
Exit

Enter
Exit
In
In
In

If your processing takes much time (per iteration) it is more reasonable to fill collection first, then process, but not yield.

Answer 3

I think David's answered the question you intended to ask (about the enumeration aspect), but two additional points to consider:

1. What would happen if ReadWriteLock.EnterReadLock threw an exception?
2. What would happen if ReadWriteLock.ExitReadLock threw an exception?

In #1, you'll call ReadWriteLock.ExitReadLock inappropriately. In #2, you may hide an existing exception that's been thrown (since finally clauses happen either because the mainline processing reached the end of the try block or because an exception was thrown; in the latter case, you probably don't want to obscure the exception). Perhaps both of those things are unlikely in this specific case, but you asked about the pattern, and as a pattern it has those issues.