0

There are many posts, votes and answers indicating using lock (this) is not a recommends pattern (not to mention a bad one).

Have a look at this one, for example.

As I'm trying to investigate this pattern a little bit, and wanted to ask whether anyone someone can think of a scenario in which using lock (this) is actually recommended, or even a must?

Community
  • 1
  • 1
Elad Gutman
  • 189
  • 1
  • 12
  • 3
    Based on the link provided.. I am unsure how you can ask this question. The answer in that link gives you the exact reason why you shouldn't lock on `this`.. what more do you need? – Simon Whitehead Oct 22 '13 at 00:27
  • @SimonWhitehead I don't think that answers the inverse though - at the start I did, but not now. This isn't asking "what issues there are", so much as "is it *ever* valid to .." and for that I cannot definitely say "no". While I have yet to run into a situation where such fits my modeling, I really want to say "it depends". – user2864740 Oct 22 '13 at 00:36
  • @user2864740 Have you ever come across a reason why "leaves you completely open to potential deadlocks" is a good design decision? Or somewhere that it is recommended? Because that is the question.. and I have never seen it recommended (nowadays anyway.. IIRC MSDN did once say "lock on the type", then changed to "lock on the current instance".. then when people realised what this was actually doing they decided on a private lock object). – Simon Whitehead Oct 22 '13 at 00:40
  • @SimonWhitehead What if a special kind of container needs to obtain a lock over a set of specialized objects it holds (for sake of argument, imagine internal type who's objects are not exposed) - why should it not lock on said objects? – user2864740 Oct 22 '13 at 00:42
  • @user2864740 I don't see how that changes anything. The wrapper can still obtain a lock over the collection it holds by locking on a private lock object. Why would you lock the current instance to sync access to an internal/private property of that instance? Maybe I misunderstand what you mean.. but I don't see how that really changes the issue. – Simon Whitehead Oct 22 '13 at 00:52
  • @elad here is another case why its a bad idea. http://haacked.com/archive/2005/04/12/neverlockthis.aspx – Daniel A. White Oct 22 '13 at 01:13
  • I can think of situations where locking on `this` is acceptable. I wouldn't go as far as saying it could ever be recommended, but there are cases, albeit limited, where it is acceptable. – Brian Gideon Oct 22 '13 at 01:20

2 Answers2

0

Rule of thumb: never lock on this but create a seperate (private) object to lock.

But... The problem is deeper: locking has a purpose, by locking you provide protection on the upper object(s) but it doesn't prevent updating the underlying objects in for instance a collection.

In most cases a lock isn't a need. Read up on the subject is what I suggest.

Multiple questions on SO cover you question. Shouldn't be hard to build an opinion about the motivation to not lock on this.

An example and pointers for further reading can be found on the blog of Phil Haack

lboshuizen
  • 2,746
  • 17
  • 20
  • Thank you for your answer, though I didn't understand the "In most cases a lock isn't a need" sentence. Regarding the last sentence of your questions - I'm asking for the complete opposite: a motivation to use lock (this), and not the motivation not to do so, as for this there are many posts. Unless everybody in the stackoverflow community thinks it's wrong... – Elad Gutman Oct 22 '13 at 01:10
  • Updated my answer with a link to a blog article – lboshuizen Oct 22 '13 at 13:36
0

Locking on THIS is evil. This means that someone may decide to lock on your instance. This means that your instance will wait until someone else releases it.

evhen14
  • 1,839
  • 12
  • 16
  • "Evil" is a fun hyperbolic word to use. It is also not constructive and alludes to a moral issue. Given controlled source (i.e. an object only used internally), why is it "evil" to wait until someone else (i.e. the controlled code) releases it? – user2864740 Oct 22 '13 at 00:40
  • @user2864740 Another developer in your team how thinks that locking on instances is better than locking on this or private fields (preferred) – evhen14 Oct 22 '13 at 00:42
  • What if the lock comes from an external "controller" container/object? Should I create a *non-private* `Lock` member such that it can lock-per-instance? (Also, that is what *code/design contracts* are for; following them is *always* crucial for correct code independent of locking or locking on a particular instance: i.e. we "all know" not to modify WinForm objects off the UI thread.) – user2864740 Oct 22 '13 at 00:44
  • @user2864740 Lock on private field in controller. If your instance needs to be lock on by external classes, there is something's wrong with how classes interact – evhen14 Oct 22 '13 at 00:46
  • Imagine the controller needs to lock on a *set* of contained objects in this case such that multiple sets - we can argue for distinctness or not - may be locked by different threads concurrently. (This is a silly example, but I feel my previous points are valid.) – user2864740 Oct 22 '13 at 00:46
  • @user2864740 So, if controller contains a set as private field, it needs a separate private field, say _locker, and use it in lock statements lock(_locker) in all methods that read/write to that set – evhen14 Oct 22 '13 at 00:50
  • If you want to make it public set, create corresponding methods (Contains, Add, Remove) and lock inside those methods, but do not expose set instance – evhen14 Oct 22 '13 at 00:51
  • @user2864740 I think your points here are valid if the OP wasn't asking about `lock`.. which is essentially `Monitor.Enter` and `Monitor.Exit`. It is usage of the sync block .. not a spin lock or some other sort of thread sync object. So there is no "waiting".. its simply locked until released. Locking on `this` opens you up to it potentially never being released within the context of `Monitor.Enter/Exit`.. but for other thread sync constructs that isn't the exact case (timeouts, etc). – Simon Whitehead Oct 22 '13 at 01:00