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?

Solution 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.

Solution 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;

Solution 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.

Solution 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.

Solution 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.

View more solutions

Comments

[Pop Catalin]

How about firing Reflector and do some IL comparison ;-).

[Marc Gravell]

@Pop: done - neither is better in IL terms - only C# style applies

[Pokus]

Very interesting, wow I learn something today!

[Vandrey]

@PopCatalin Im sorry to ask this, but what is "IL" and Reflector?

[Peter]

@Sunburst275: Have a look at microsoft.com/en-us/p/ilspy/…

[Marc Gravell]

Well, that is true of the try/finally - however, the return outside the lock still requires extra locals which can't be optimised away - and takes more code...

[Meydjer Luzzoli]

You can't return from a try block; it must end with a ".leave" op-code. So the CIL emitted should be the same in either case.

[Marc Gravell]

You are right - I've just looked at the IL (see updated post). I learnt something ;-p

[Meydjer Luzzoli]

Cool, unfortunately I learned from painful hours trying to emit .ret op-codes in try blocks and having the CLR refuse to load my dynamic methods :-(

[Marc Gravell]

I used the (free and excellent) Red Gate's .NET Reflector (was: Lutz Roeder's .NET Reflector), but ILDASM would do too.

[Marc Gravell]

One of the most powerful aspects of Reflector is that you can actually disassemble IL to your preferred language (C#, VB, Delphi, MC++, Chrome, etc)

[Marc Gravell]

I can relate; I've done a fair amount of Reflection.Emit, but I'm lazy; unless I'm very sure about something, I write representative code in C# and then look at the IL. But it is surprising how quickly you start thinking in IL terms (i.e. sequencing the stack).

[Torbjørn]

This is correct, a point the other responders seem to be missing. The simple samples they have made may produce the same IL, but this is not so for most real-life scenarios.

[Torbjørn]

For your simple example the IL stays the same, but that is probably because you only return a constant value?! I believe for real life scenarios the result may differ, and parallel threads may cause problems for each other by modifying the value before it's returned it the return statement is outside of the lock block. Dangerous!

[Raheel Khan]

@MarcGravell: I just came across your post while pondering the same and even after reading your answer, I'm still not sure about the following: Are there ANY circumstances where using the outside approach could break thread-safe logic. I ask this since I prefer a single point of return and don't 'FEEL' good about its thread-safety. Although, if the IL is the same, my concern should be moot anyways.

[Marc Gravell]

@RaheelKhan no, none; they are the same. At the IL level, you cannot ret inside a .try region.

[Akshat Agarwal]

Im surprised the other answers dont talk about this

[ViRuSTriNiTy]

@Torbjørn I think your concerns are valid but only when Program, ReturnInside and ReturnOutside are declared static, otherwise it's safe as i would be a local variable then.

[Guillermo Ruffino]

In this sample they are talking about using a stack variable to store the return value, i.e. only the return statement outside the lock and of course the variable declaration. Another thread should have another stack and thus could not make any harm, am I right?

[Alberto Rivelli]

If you return inside the lock, what is returned if the thread waiting to enter the monitor is interrupted?

[Marc Gravell]

@Alberto again, the point of the IL dump is that they are identical; so... the same thing

[Alberto Rivelli]

yes sorry I'm not comfortable reading IL, I got the answer from a duplicated question. Thanks anyway

[Uroš Joksimović]

I don't think this is a valid point, as another thread could update the value between the return call and the actual assignment of the return value to the variable on the main thread. The value being returned can't be changed or or be guaranteed consistency with the current actual value either way. Right?

[Theodor Zoulias]

This answer is incorrect. Another thread cannot change a local variable. Local variables are stored in the stack, and each thread has its own stack. Btw the default size of a thread's stack is 1 MB.

[Theodor Zoulias]

Regarding the current state of your answer (revision 13), you are still speculating about the reasons of the existence of the lock, and deriving meaning from the placement of the return statement. Which is a discussion unrelated to this question IMHO. Also I find the usage of "misrepresents" quite disturbing. If returning from a lock misrepresents the flow of control, then the same could be said for returning from a try, catch, using, if, while, for, and any other construct of the language. It's like saying that the C# is riddled with control flow misrepresentations. Jesus...

[mklement0]

"It's like saying that the C# is riddled with control flow misrepresentations" - Well, that is technically true, and the term "misrepresentation" is only a value judgment if you choose to take it that way. With try, if, ... I personally don't tend to even think about it, but in the context of lock, specifically, the question arose for me - and if it didn't arise for others too, this question would never have been asked, and the accepted answer wouldn't have gone to great lengths to investigate the true behavior.

[mklement0]

@TheodorZoulias If the local variable contains a reference (a "pointer" to an instance of a reference type), another thread can indeed come in and modify the referenced object after leaving the lock block. However, I agree that this ultimately doesn't matter, because if such a referenced object needs protection when accessed by the caller, the only way to guarantee that is with a lock on the caller's side - in which case it doesn't matter how many method-internal statements lie between the end of the lock block and the return statement.