CODEWITHSUNDEEP
.netthread-safety
Quick Joe Smith
Quick Joe Smith

Reputation: 8222

Is locking on the requested object a bad idea?

Most advice on thread safety involves some variation of the following pattern:

public class Thing
{
    private readonly object padlock = new object();

    private IDictionary stuff, andNonsense;

    public Thing()
    {
        this.stuff = new Dictionary();
        this.andNonsense = new Dictionary();
    }

    public IDictionary Stuff
    {
        get
        {
            lock (this.padlock)
            {
                if (this.stuff.Count == 0)
                    this.stuff = this.SomeExpensiveOperation();
                return this.stuff;
            }
        }
    }
    
    public IDictionary AndNonsense
    {
        get
        {
            lock (this.padlock)
            {
                if (this.andNonsense.Count == 0)
                    this.andNonsense = this.AnotherExpensiveOperation();
                return this.andNonsense;
            }
        }
    }
    // Rest of class...
}

In cases where the get operations are expensive and unrelated, a single locking object is unsuitable because a call to Stuff would block all calls to AndNonsense, degrading performance. And rather than create a lock object for each call, wouldn't it be better to acquire the lock on the member itself (assuming it is not something that implements SyncRoot or somesuch for that purpose? For example:

public IDictionary Stuff
{
    get
    {
        lock (this.stuff)
        {
            if (this.stuff.Count == 0)
                this.stuff = this.SomeExpensiveOperation();
            return this.stuff;
        }
    }
}

Strangely, I have never seen this approach recommended or warned against. Am I missing something obvious?

EDIT 24 May 2010

I have made some extensive changes because I really borked my example. Serves me right for trying to simplify the example too much for clarity.

Summary of edits:

  • Locking object in first example is no longer static (my original example was from a static class).
  • Fields/properties no longer a string, and initialised in constructor so never null.
  • Moved return statement inside the lock{...} block.

Upvotes: 1

Views: 158

Answers (6)

MaLio
MaLio

Reputation: 2540

Never lock on types or strings, or other objects that may potentially be shared accorss multiple appdomains. Locking on a an instance field as such is no problem, just ensure that your instance has exclusive access to it.

Upvotes: 0

JoeGeeky
JoeGeeky

Reputation: 3796

In this scenario, I would use ReaderWriterLockSlim which allows you to have multiple overlapping 'Read' locks by only completely locking when you enter a 'Write' lock either directly or via an upgrade lock. Alternatively, you can go lock-free by using Thread.MemoryBarrier, but this is tricky and as an advanced technique will require some specialized tests to make sure it actually works.

Upvotes: 3

Neil Moss
Neil Moss

Reputation: 6838

As a note, be aware that your examples are not intrinsically thread safe, as you are querying the value of the variable you are trying to protect from outside the lock.

Another thread can get a slice, just after the first thread running Stuff {get;} releases the lock, and sets the value of this.stuff to null again before the first thread actually returns this.stuff

Upvotes: 1

AakashM
AakashM

Reputation: 63348

As well the problem that @codeka points out, note also that lock() requires a reference type, so your proposed system would have to come up with a different approach for value types.

Upvotes: 0

Jon Skeet
Jon Skeet

Reputation: 1500893

Your version of the "standard advice" would be very bad - it would use a single lock for the whole AppDomain. Normally the lock object is an instance variable, not a static one.

Now, as for your suggestion: even worse idea, IMO - because you don't know what else will be locking on it. Some other code may be using the same string - and likewise locking on it. Now suppose two threads acquire two locks instead of one. When you've no idea what's locking on what, you have no hope of avoiding deadlock other than dumb luck. (There's also the nullity issue mentioned by codeka.)

Additionally, this starts to be confusing if the value of the variable changes - you would then potentially have two threads executing inside the lock block at the same time, which presumably you were trying to avoid.

Wherever possible, you should (IMO) lock on something that only your code knows about. While that in itself won't keep you deadlock free or safe, it's a good starting point as it makes it much easier to reason about your code. There's nothing to say that has to be a single lock per instance - you can create finer-grained locks if you want - but keep them private to yourself if at all possible.

Upvotes: 4

Dean Harding
Dean Harding

Reputation: 72658

Well, I assume you haven't actually tried this because lock(null) will throw an ArgumentNullException, so you can't actually do it like that.

You could use a separate "stuffLock" object, like so:

class Thing
{
    private string stuff;
    private object stuffLock = new Object();

    public string Stuff
    {
        get
        {
            lock(stuffLock)
            {
                if (stuff == null)
                    stuff = "Still threadsafe";
            }
            return stuff;
        }
    }
}

Upvotes: 3

Related Questions