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?

score 7 · Accepted Answer · edited May 23 '17 at 11:55

7

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.

edited May 23 '17 at 11:55

Community

1
1

answered Jun 02 '10 at 10:26

David Hedlund

128,221
31
203
222

2

What if somebody does not use foreach but makes a single call to MoveNext() and Current? – codymanix Jun 02 '10 at 11:47
1

@David, @Martin: This answer is misleading and codymanix is correct. A simple one-liner -- `GetNumbers().GetEnumerator().MoveNext();` -- will display "Start" and then nothing else (and if you were using locking in your example then your lock would never be released). – LukeH Jun 02 '10 at 12:05
@codymanix, @LukeH: this is true. i didn't think of that scenario. @Martin: i'd opt to delete this answer as it is misleading, as others have pointed out, altho i can't, since this is accepted. if you untick the accept-box, i'll delete this post, so as to not mislead anybody else – David Hedlund Jun 02 '10 at 12:22
I have unaccepted the answer as you asked. Personally I would just update the answer to reflect this issue. In reality the way I am actually using this enumerator is as a generator that never finishes, so I have already fixed this issue. – Martin Jun 03 '10 at 13:54

score 3 · Answer 2 · edited May 23 '17 at 11:47

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

What would happen if ReadWriteLock.EnterReadLock threw an exception?
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.

Hmm, those are good points. number 1 just caught me out elsewhere and I shall update my sample above to reflect how I fixed that. — Martin, Jun 02 '10 at 10:39

score 2 · Answer 3 · answered Jun 02 '10 at 10:34

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.

Is this a dangerous locking pattern?

3 Answers3