Using lock(obj) inside a recursive call

Question

As per my understanding a lock is not released until the runtime completes the code block of the lock(obj) ( because when the block completes it calls Monitor.Exit(obj).

With this understanding i am not able to understand the reason behind the behaviour of the following code :

private static string obj = "";
        private static void RecurseSome(int number)
        {
            Console.WriteLine(number);
            lock (obj)
            {
                RecurseSome(++number);
            }
        }

//Call: RecurseSome(0)

//Output: 0 1 2 3...... stack overflow exception

There must be some concept that i am missing. Please help.

score 47 · Accepted Answer · edited Oct 31 '13 at 05:46

A lock knows which thread locked it. If the same thread comes again it just increments a counter and does not block.

So, in the recursion, the second call also goes in - and the lock internally increases the lock counter - because it is the same thread (which already holds the lock).

ms-help://MS.VSCC.v90/MS.MSDNQTR.v90.en/dv_csref/html/656da1a4-707e-4ef6-9c6e-6d13b646af42.htm

Or MSDN: http://msdn.microsoft.com/en-us/library/c5kehkcz.aspx

states:

The lock keyword ensures that one thread does not enter a critical section of code while another thread is in the critical section. If another thread tries to enter a locked code, it will wait, block, until the object is released.

Note the thread references and the emphasis on "ANOTHER" thread.

Steven · Answer 2 · 2013-10-31T09:19:42.277

38

Please do NOT lock on a string object. This could lead to unexpected behavior such as deadlocks in your application. You are currently locking on the empty string, which is even worse. The whole assembly is using the same empty string. And to make things worse; as an optimization, the CLR reuses strings over AppDomains. Locking on string means you are possibly doing a cross-domain lock.

Use the following code as lock object:

private readonly static object obj = new object();

UPDATE

In fact, I think it's safe to say that being allowed to lock on anything is a major design flaw in the .NET framework. Instead, they should have created some sort of SyncRoot sealed class and only allowed the lock statement and Monitor.Enter to accept instances of SyncRoot. This would have saved us a lot of misery. I do understand where this flaw is coming from though; Java has the same design.

edited Oct 31 '13 at 09:19

answered Mar 09 '10 at 08:19

Steven

166,672
24
332
435

6

+1 for being the only one to pick this up. Strings can be interned, which means weird behavior. – Richard Szalay Mar 09 '10 at 08:40
1

It especially means uncontrolled behavior - totally correct. Use anything (object, assign a Guid, an int, anything) just NOT a string. – TomTom Jun 20 '12 at 07:37
In the above example, fro example., you are like 99% sure to lock String.Empty - the interned emty string. Do that on 2 plces and you get side effects and spend days finding them. – TomTom Jun 20 '12 at 07:38
6

@TomTom: Don't use a Guid or an int! Think carefully about what will happen when they are passed to Monitor.Enter. – Eric Lippert Oct 31 '13 at 06:14
2

@Steven: I agree with you that the ability to lock anything, and the attendant increase in memory usage for making pointers to sync blocks that are never, ever used, has serious drawbacks. I don't know that I would say it is a major flaw, but I take your point. Your understanding of the fundamental cause is correct; the CLR has several questionable design choices that make more sense when you realize that the CLR designers wanted to be able to support a superset of Java-like behaviours. This is where we get unsafe array covariance as well. – Eric Lippert Oct 31 '13 at 14:46
1

The advice to not lock on a string object shouldn't be taken as an absolute mandate. There are scenarios where locking on an interned string can save you from having to manage mutex objects manually. I talk about this here: http://stackoverflow.com/a/29878495/64334 – Ronnie Overby Sep 14 '16 at 01:02

Steven · Answer 3 · 2010-03-09T08:33:29.440

As others already noted, the lock is helt by the thread and will therefore work. However, I want to add something to this.

Joe Duffy, a concurrency specialist from Microsoft, has multiple design rules about concurrency. One of his design rules state:

9 . Avoid lock recursion in your design. Use a non-recursive lock where possible.

Recursion typically indicates an over-simplification in your synchronization design that often leads to less reliable code. Some designs use lock recursion as a way to avoid splitting functions into those that take locks and those that assume locks are already taken. This can admittedly lead to a reduction in code size and therefore a shorter time-to-write, but results in a more brittle design in the end.

(source)

To prevent the recursive lock, rewrite the code to the following:

private readonly static object obj = new object();

private static void Some(int number)
{
    lock (obj)
    {
        RecurseSome(number);
    }
}

private static void RecurseSome(int number)
{
    Console.WriteLine(number);
    RecurseSome(++number);
}

Furthermore, I your code will throw a StackOverflowException, because it never ends recursively calling itself. You could rewrite your method as follows:

private static void RecurseSome(int number)
{
    Console.WriteLine(number);
    if (number < 100)
    {
        RecurseSome(++number);
    }
}

+1 for the tip on recursive lock design. I've never thought about it like that before - will keep this in mind for future code. — AnorZaken, Sep 25 '15 at 12:25

score 7 · Answer 4 · answered Mar 09 '10 at 08:07

7

The lock is owned by the current thread. The recursive call is made on the current thread too. If another thread tries to acquire the lock, it'll block.

answered Mar 09 '10 at 08:07

Mehrdad Afshari

414,610
91
852
789

Paul Alexander · Answer 5 · 2010-03-09T08:17:45.000

If you're asking about the stack overflow exception - it's because there's nothing in there to break from the recursion. The stack space is usually only a few K and you'll exhaust the space pretty quick.

Now the lock in this case can be used to serialize the output of the call so that if you call RecurseSome from two different threads, you'll see the entire list from the first thread, followed by the entire list from the second thread. Without the lock, the output from the two threads would be interleaved.

You could achieve the same results without taking the lock recursively by splitting the method:

private static void RecurseSome(int number)
{
    lock (obj)
    {
        RecurseSomeImp(number);
    }
}

private static void RecurseSomeImp(int number)
{
    Console.WriteLine(number);
    if( number < 100 ) // Add a boundary condition
        RecurseSomeImp(++number);
}

This will actually perform better as well as taking and releasing locks are fast, but not free.

score -1 · Answer 6 · answered Mar 09 '10 at 08:08

-1

It has nothing to do with lock. Check your recursion code. where is the boundary case to stop the recursion?

answered Mar 09 '10 at 08:08

Fakrudeen

5,778
7
44
70

The people who downvoted can explain? He is asking the reason for stackoverflow [no pun intended]. It will happen irrespective of the lock with that code. So lock is red herring. – Fakrudeen Mar 09 '10 at 08:25
1

It is obvious his code is there to demonstrate the problem. He THOUGHT it should not recurse but block at the recursion. – TomTom Mar 09 '10 at 08:29
it is not obvious. >>>the reason behind the behaviour of the following code. behaviour = stackoverflow reason=no boundary case. If he cared only about reentry of lock, he could have used lock(obj);lock(obj); or f(){lock(obj){}; g();} g(lock(obj){}); Why introduce recursion [that too with no boundary case] as red herring. Also look at the replies of others. Many of them have the same confusion. – Fakrudeen Mar 09 '10 at 09:01
@Fakrudeen You are putting words in the OP's mouth by saying he is asking for the reason for stackoverflow. The OP makes it clear he is querying an understanding about locking behaviour not infinite looping. The title of the question is also clear that it is about recursion. That is why you were downvoted. – Paul Childs Apr 06 '21 at 23:40

score -1 · Answer 7 · answered Mar 09 '10 at 08:13

-1

it is a continuous loop because there's no way to determine when to stop the recursion and the object that the thread is trying to access is always blocked.

answered Mar 09 '10 at 08:13

hallie

2,760
19
27

Self contradictory. – Paul Childs Apr 06 '21 at 23:36

Using lock(obj) inside a recursive call

7 Answers7