61

Is there any difference between this:

internal class MyClass
{
    private readonly object _syncRoot = new Object();

    public void DoSomething() 
    {
        lock(_syncRoot)
        {
            ...
        }
    }

    public void DoSomethingElse() 
    {
        lock(_syncRoot)
        {
            ...
        }
    }
}

and this:

internal class MyClass
{
    [MethodImpl(MethodImplOptions.Synchronized)]
    public void DoSomething() 
    {
        ...
    }

    [MethodImpl(MethodImplOptions.Synchronized)]
    public void DoSomethingElse() 
    {
        ...
    }
}

The only difference I see is that the first approach locks on some private member whereas the second approach locks on the instance itself (so it should lock everything else in the instance). Is there any general advice which approach to use? I have currently found two classes with similar purpose in our project each written with different approach.

Edit:

Perhaps one more question. Is this:

internal class MyClass
{
    [MethodImpl(MethodImplOptions.Synchronized)]
    public void DoSomething() 
    {
        ...
    }
}

exactly same like this:

internal class MyClass
{
    public void DoSomething() 
    {
        lock(this) 
        {
            ...
        }
    }
}
Ladislav Mrnka
  • 360,892
  • 59
  • 660
  • 670
  • 4
    check out http://stackoverflow.com/questions/541194/c-version-of-javas-synchronized-keyword – Winston May 26 '11 at 14:24
  • @Winston: Great. My question is almost a duplicate. – Ladislav Mrnka May 26 '11 at 14:27
  • I would view the attribute form as a "polite suggestion" to the compiler. I'm not sure if they are bound by the standard to follow it. Whereas explicitly using a `lock` statement enforces a compile time requirement. – user7116 May 26 '11 at 14:37
  • 1
    @sixlet A polite suggestion wouldn't be very usable/reliable, would it? – H H May 26 '11 at 19:49
  • 2
    @Henk: I'm just saying I don't remember it being locked down by any standard as "required to be implemented" by a compliant CLR. – user7116 May 26 '11 at 19:54

5 Answers5

43

The first method is preferred because you can (and should) make _syncRoot private. This lowers the risk of deadlocking.

The MethodImplOptions.Synchronized is a left-over from an earlier ambitious idea that turned out to be not so good after all.

Regarding the last question: Yes, according to this blog they are functionally equivalent (but not implemented the same way). And all forms of lock(this) are discouraged, again because of deadlock scenarios.

H H
  • 263,252
  • 30
  • 330
  • 514
  • 3
    The public availability among others. – H H May 26 '11 at 14:49
  • 1
    Indeed the fact that anyone can lock is the concern mentioned here http://blogs.msdn.com/b/bclteam/archive/2004/01/20/60719.aspx – ubershmekel Nov 04 '15 at 02:13
  • What about using 'MethodImplOptions.Synchronized' internally, for **`private`** methods only? – samus Mar 15 '18 at 18:45
  • 1
    @samsara The same 'issue' exists, in that a lock on the *instance* (or Type, for static methods) is acquired, regardless of method visibility. If some "other, bad code" *also* obtains a lock on the instance then it *could* result in unexpected locking, or even deadlocks; hence the "best practice" suggestion is that that *"all"* lock objects - except where such is purposefully exposed (like SyncRoot), as opposed to exposed via unexpected side-effect/bleed - are hidden away from the outside world. – user2864740 Jul 22 '18 at 18:34
12

check out http://blogs.msdn.com/b/bclteam/archive/2004/01/20/60719.aspx and http://www.experts-exchange.com/Programming/Languages/C_Sharp/Q_20926988.html
They discuss about lock(this) too and discourage using it since:

completely unrelated code can choose to lock on that object as well

Quoting from EE:

If you lock an object, all other threads that need to access THIS PARTICULAR OBJECT will wait, until the other object finishes. However if you mark a method as Synchronized, THIS PARTICULAR METHOD will not be executed at more than one thread. Lock secures the object, Synchronized secures the method.

Kamyar
  • 18,639
  • 9
  • 97
  • 171
  • Yes I know about this issue. It is directly pointed in MSDN that Synchronized methods should not be used on public classes. – Ladislav Mrnka May 26 '11 at 14:31
  • 2
    +1 for the second quote. This is the most important distinction between the two mechanisms IMO. – Jake T. May 27 '11 at 00:06
  • "THIS PARTICULAR METHOD will not be executed at more than one thread" WIN FOR ME , less code , easy , 1 line ... beautiful ! just what I need . If i have many threads running wild , lock() will become hell – Zakos Nov 15 '12 at 16:55
  • 1
    The quote from EE is wrong. According the the [documentation](https://learn.microsoft.com/en-us/dotnet/api/system.runtime.compilerservices.methodimploptions), the `Synchronized` means: *The method can be executed by only one thread at a time. Static methods lock on the type, whereas instance methods lock on the instance. Only one thread can execute in any of the instance functions, and only one thread can execute in any of a class's static functions.* So no, it has nothing to do with THIS PARTICULAR METHOD. While a thread executes `MethodA`, another thread can't enter `MethodB`. – Theodor Zoulias Jun 19 '20 at 10:59
8

Just having a quick look and found that portable devices do not support MethodImplOptions.Synchronized.

There is also a remark:

Locking on the instance or on the type, as with the Synchronized flag, is not recommended for public types, because code other than your own can take locks on public types and instances. This might cause deadlocks or other synchronization problems.

source: http://msdn.microsoft.com/en-us/library/system.runtime.compilerservices.methodimploptions%28v=VS.100%29.aspx

Wojtek Turowicz
  • 4,086
  • 3
  • 27
  • 38
2

I think the difference would depend on what objects are being referenced in the decorated methods. From what I've read, the decoration actually implements lock() in IL.

The best approach would be to do the most specific locking as necessary.

Steve Mallory
  • 4,245
  • 1
  • 28
  • 31
1

This article may help you: http://www.yoda.arachsys.com/csharp/threads/lockchoice.shtml

Generally I would avoid locking on 'this', as private lock variables, provide better control. I would recommend locking on 'this', if it's a custom collection class, something along the lines of SyncRoot, if that is what is required.

Hasanain

Hasanain
  • 925
  • 8
  • 16