Should a return statement be inside or outside a lock?

Question

I just realized that in some place in my code I have the return statement inside the lock and sometime outside. Which one is the best?

1)

void example()
{
    lock (mutex)
    {
    //...
    }
    return myData;
}

2)

void example()
{
    lock (mutex)
    {
    //...
    return myData;
    }

}

Which one should I use?

Answer 1

Essentially, which-ever makes the code simpler. Single point of exit is a nice ideal, but I wouldn't bend the code out of shape just to achieve it... And if the alternative is declaring a local variable (outside the lock), initializing it (inside the lock) and then returning it (outside the lock), then I'd say that a simple "return foo" inside the lock is a lot simpler.

To show the difference in IL, lets code:

static class Program
{
    static void Main() { }

    static readonly object sync = new object();

    static int GetValue() { return 5; }

    static int ReturnInside()
    {
        lock (sync)
        {
            return GetValue();
        }
    }

    static int ReturnOutside()
    {
        int val;
        lock (sync)
        {
            val = GetValue();
        }
        return val;
    }
}

(note that I'd happily argue that ReturnInside is a simpler/cleaner bit of C#)

And look at the IL (release mode etc):

.method private hidebysig static int32 ReturnInside() cil managed
{
    .maxstack 2
    .locals init (
        [0] int32 CS$1$0000,
        [1] object CS$2$0001)
    L_0000: ldsfld object Program::sync
    L_0005: dup 
    L_0006: stloc.1 
    L_0007: call void [mscorlib]System.Threading.Monitor::Enter(object)
    L_000c: call int32 Program::GetValue()
    L_0011: stloc.0 
    L_0012: leave.s L_001b
    L_0014: ldloc.1 
    L_0015: call void [mscorlib]System.Threading.Monitor::Exit(object)
    L_001a: endfinally 
    L_001b: ldloc.0 
    L_001c: ret 
    .try L_000c to L_0014 finally handler L_0014 to L_001b
} 

method private hidebysig static int32 ReturnOutside() cil managed
{
    .maxstack 2
    .locals init (
        [0] int32 val,
        [1] object CS$2$0000)
    L_0000: ldsfld object Program::sync
    L_0005: dup 
    L_0006: stloc.1 
    L_0007: call void [mscorlib]System.Threading.Monitor::Enter(object)
    L_000c: call int32 Program::GetValue()
    L_0011: stloc.0 
    L_0012: leave.s L_001b
    L_0014: ldloc.1 
    L_0015: call void [mscorlib]System.Threading.Monitor::Exit(object)
    L_001a: endfinally 
    L_001b: ldloc.0 
    L_001c: ret 
    .try L_000c to L_0014 finally handler L_0014 to L_001b
}

So at the IL level they are [give or take some names] identical (I learnt something ;-p). As such, the only sensible comparison is the (highly subjective) law of local coding style... I prefer ReturnInside for simplicity, but I wouldn't get excited about either.

Answer 2

It doesn't make any difference; they're both translated to the same thing by the compiler.

To clarify, either is effectively translated to something with the following semantics:

T myData;
Monitor.Enter(mutex)
try
{
    myData= // something
}
finally
{
    Monitor.Exit(mutex);
}

return myData;

Answer 3

I would definitely put the return inside the lock. Otherwise you risk another thread entering the lock and modifying your variable before the return statement, therefore making the original caller receive a different value than expected.

Answer 4

If think the lock outside looks better, but be careful if you end up changing the code to:

return f(...)

If f() needs to be called with the lock held then it obviously needs to be inside the lock, as such keeping returns inside the lock for consistency makes sense.

Answer 5

It depends,

I am going to go against the grain here. I generally would return inside of the lock.

Usually the variable mydata is a local variable. I am fond of declaring local variables while I initialize them. I rarely have the data to initialize my return value outside of my lock.

So your comparison is actually flawed. While ideally the difference between the two options would be as you had written, which seems to give the nod to case 1, in practice its a little uglier.

void example() { 
    int myData;
    lock (foo) { 
        myData = ...;
    }
    return myData
}

vs.

void example() { 
    lock (foo) {
        return ...;
    }
}

I find case 2 to be considerably easier to read and harder to screw up, especially for short snippets.

Answer 6

For what it's worth, the documentation on MSDN has an example of returning from inside of the lock. From the other answers on here, it does appear to be pretty similar IL but, to me, it does seem safer to return from inside the lock because then you don't run the risk of a return variable being overwritten by another thread.

Answer 7

To make it easier for fellow developers to read the code I would suggest the first alternative.

Answer 8

lock() return <expression> statements always:

1) enter lock

2) makes local (thread-safe) store for the value of the specified type,

3) fills the store with the value returned by <expression>,

4) exit lock

5) return the store.

It means that value, returned from lock statement, always "cooked" before return.

Don't worry about lock() return, do not listen to anyone here ))

Answer 9

^{Note: I believe this answer to be factually correct and I hope that it is helpful too, but I'm always happy to improve it based on concrete feedback.}

To summarize and complement the existing answers:

• The accepted answer shows that, irrespective of which syntax form you choose in your C# code, in the IL code - and therefore at runtime - the return doesn't happen until after the lock is released.

  • Even though placing the return inside the lock block therefore, strictly speaking, misrepresents the flow of control^[1], it is syntactically convenient in that it obviates the need for storing the return value in an aux. local variable (declared outside the block, so that it can be used with a return outside the block) - see Edward KMETT's answer.

• Separately - and this aspect is incidental to the question, but may still be of interest (Ricardo Villamil's answer tries to address it, but incorrectly, I think) - combining a lock statement with a return statement - i.e., obtaining value to return in a block protected from concurrent access - only meaningfully "protects" the returned value in the caller's scope if it doesn't actually need protecting once obtained, which applies in the following scenarios:

  • If the returned value is an element from a collection that only needs protection in terms of adding and removing elements, not in terms of modifications of the elements themselves and/or ...

  • ... if the value being returned is an instance of a value type or a string.

    • Note that in this case the caller receives a snapshot (copy)^[2] of the value - which by the time the caller inspects it may no longer be the current value in the data structure of origin.

  • In any other case, the locking must be performed by the caller, not (just) inside the method.

---

^{[1] Theodor Zoulias points out that that is technically also true for placing return inside try, catch, using, if, while, for, ... statements; however, it is the specific purpose of the lock statement that is likely to invite scrutiny of the true control flow, as evidenced by this question having been asked and having received much attention.}

^{[2] Accessing a value-type instance invariably creates a thread-local, on-the-stack copy of it; even though strings are technically reference-type instances, they effectively behave like-value type instances.}