lock(new object()) -- Cargo cult or some crazy "language special case"?

Question

I'm reviewing some code written by a consultant, and while dozens of red flags have already popped up, I can't wrap my head around the following snippet:

private void foo()
{
    if (InvokeRequired)
    {
        lock (new object())
        {
            if (m_bar!= null)
                Invoke(new fooDelegate(foo), new object[] { });
        }
    }
    else
    {
        if(OnBazChanged != null)
            OnBazChanged();
    }
}

What is lock(new object()) doing here? Should have no effect whatsoever as it's always locking on another object, but this kind of locking is persistent throughout the code, even in non-copy-and-pasted parts. Is this some special case in the C# language that is compiled to something I don't know about, or has the programmer simply adopted some cargo cult that happened to work some time ago?

I think they're very confused. They probably saw it where the `new object()` was stored in a field, and that field was used in the `lock()` statements, and they didn't know better not to inline it. — Damien_The_Unbeliever, Aug 20 '12 at 07:39
That "consultant" has some explaining to do... you aren't wrong: that `lock` code is entirely useless — Marc Gravell, Aug 20 '12 at 07:48
@Baboon: Only if you're not the one who has to do the refactoring... — , Aug 20 '12 at 07:51
Except that this is plain wrong it seems that threading is becoming the best the excuse to write code nobody understands. — Alois Kraus, Aug 20 '12 at 07:54
Plus, if this is WinForms, then I can't see why there should be a lock there at all. — Drew Noakes, Aug 20 '12 at 08:07
Remove it, then re-run your 100% code-coverage test suite. What's that? The previous consultant didn't make one? — Spacedman, Aug 20 '12 at 13:14
Had it been Java, rather than .NET, you could do something like this with the `synchronized` keyword to piggy-back on the specific barriers that are emitted. However, in the exceedingly rare case that people do piggy-backing, they tend to do it with `volatile` reads and writes. Using `lock` or `synchronized` like this is almost certainly a bug. — Chris Vest, Aug 20 '12 at 13:23
I charge €40/hr - is there, by a any chance, a job available now? — Martin James, Aug 21 '12 at 10:30
Your ex-consultant probably knows this code is totally useless but sticks it in everywhere he can to make people think maybe they've missed somethingand they are great - just like you originally asked. The reality is that most managers do not review code written by developers. They almost expect one developer/consultant to disagree with their predecessor's coding. This code compiles and never produces any errors at design, compile or runtime. You probably ran something like ReSharper on it and it flagged this problem up. The consulting/contracting market is awash with people like this. — Lee James, Aug 21 '12 at 19:30
I've haid this problem with 'consultants'. When I started on job X, I found a massive test phase to ensure that all inter-processor connections were shut down during the periodic backup to a peripheral drive, crippling performance and slowing development to a crawl. It took about 10 minutes to spot the C 'read/modify/write' of a spinlock with the other processor and another 10 to replace it with an assembler increment. This completely fixed the issue but I was nearly fired for modifying code I had no authority to fix. The developers had been using the huge gob of crap code for two years.. — Martin James, Aug 23 '12 at 08:57
@lbruder - it's still funny if you're a consultant too. Those kind of people used to keep me in business. — Chuck Dee, Aug 23 '12 at 18:48
That consultant should never do programing, not just threading, programing itself. Damn that's like cancer. — Everyone, Mar 09 '17 at 22:08

score 83 · Accepted Answer · edited Aug 20 '12 at 13:43

83

I wouldn't be suprised if it was someone who saw this:

private readonly object lockObj = new object();

private void MyMethod()
{
    lock(lockObj)
    {
        // do amazing stuff, so amazing it can only run once at a time
        // e.g. comands on the Mars Rover, or programs on iOS pre 4 / 5 ??
    }
}

and thought he could cut the number of lines.

I'd be very worried if that were the case though...

edited Aug 20 '12 at 13:43

sehe

328,274
43
416
565

answered Aug 20 '12 at 07:38

cjk

43,338
9
74
109

4

He might saw a method called "newObject()" and this method returned a singleton instance, but he said "hey, doesn't c# have keywords for that"? – Amiram Korach Aug 20 '12 at 07:42
9

It indeed sounds like a refactoring job without effectively understanding what was goeing on. – Myrtle Aug 20 '12 at 07:44
Considering all the other WTFs I found in the code (and the number of times the program crashed with some exception), I'm afraid that's most probably the answer... – Aug 20 '12 at 07:49
@lbruder Hopefully you can revert to before he started breaking everything and just start again. – OrangeDog Aug 20 '12 at 09:59
1

@OrangeDog: Sadly, that is impossible, as the code concerned was written before I joined the company. Now that there are changes to make, maybe I can convince Management to let me fix the code. Otherwise I will not take responsibility for any instabilities (the last one to touch anything is the one to blame)... – Aug 20 '12 at 12:05
2

@Ibruder Higher-priority would be to convince Management that they need version control, automated testing and review systems. If the last one to touch anything is the one to blame, then it doesn't sound like a very good company to be working for. – OrangeDog Aug 20 '12 at 14:10
May also be worth convincing management that whoever wrote this should be digging ditches instead of writing code. This is professional negligence. – FMM Aug 20 '12 at 15:07
1

I don't care much about the obviously broken locking - everyone else has already pointed it out, but +1 just for for 'or programs on iOS pre 4 / 5' – Martin James Aug 21 '12 at 10:25
@MartinJames - that's what I was going for :) – cjk Aug 21 '12 at 11:13
Shouldn't be the object static? – abatishchev May 12 '15 at 21:04

score 15 · Answer 2 · answered Aug 20 '12 at 07:39

Here is similar question, and answer:

Locks ensure mutual exclusion -- no more than one thread may hold the lock at the same time. The lock is identified with a specific object instance. You are creating a new object to lock on every time and you don't have any way to inform any other thread to lock on that exact same object instance. Therefore, your locking is useless.

score 2 · Answer 3 · answered Sep 06 '12 at 02:21

2

It is probably useless. But there is the off chance it is there to create a memory barrier. Not sure if c# does lock elision or if it does whether it preserves the ordering semantics of the lock.

answered Sep 06 '12 at 02:21

benmmurphy

2,461
1
19
28

It was a few year ago but man, you totally deserve +1 for stating this obvious fact that some people completely ignored. For example, on Universal Windows Platform there is no MemoryBarrier() method and magic with Interlocked.CompareExchange and lock(new Object()) become the only ways to deal with some problems. – Sergey.quixoticaxis.Ivanov Dec 28 '16 at 17:31
I didn't even know C# allowed this. Great of you to point it out. – Everyone Apr 20 '18 at 13:24

lock(new object()) -- Cargo cult or some crazy "language special case"?

3 Answers3