Click here to Skip to main content
Click here to Skip to main content

Tagged as

Go to top

Smarter than lock(), Cleaner than TryEnter

, 11 Apr 2014
Rate this:
Please Sign up or sign in to vote.
Lock has pitfalls: let's use Monitor.TryEnter with less boilerplate

Introduction

Locking is important for synchronizing access to data shared between threads. Ideally, you want to avoid the need for this but sometimes it is not possible or practical. So instead, we must lock - and unlock - correctly, and ideally without cluttering up the codebase.

C# provides us the lock(...) {} statement. Trouble is, this has pitfalls: notably because it blocks the current thread until it actually acquires the lock. If another thread has locked an object and (for example) is stuck in an infinite loop, when you attempt to lock the same object, you will wait on it forever because lock does not time out - and the application will be deadlocked.

In my opinion, it is better to use timeouts so your code will not wait forever if something goes wrong, so a better strategy is to use Monitor.TryEnter, and include a maximum timeout (you did declare a constant for it, instead of hardcoding - right?). Then, you add a try/finally so you will always call Monitor.Exit afterward.

All told, you will end up with a lot of surrounding code like this:

// at class-level
private readonly object syncObject = new object();
private const int TimeoutMaxWait = 50; // milliseconds

// at each location you require a lock
if (Monitor.TryEnter(syncObject, TimeoutMaxWait))
{
    try
    {
        // useful work
    }
    finally
    {
        Monitor.Exit(syncObject);
    }
}
else
{
    // failed to get lock: throw exceptions, log messages, get angry etc.
}  

Now, there is nothing wrong with this, but if we have a class with a lot of methods that require locking, we will have a lot of this boilerplate code. Maybe we could do something about it, and reduce the above code to:

using (Lock())
{
    // useful work
} 

Using the Code

Whether this works for you is heavily dependent on what you do when it fails to lock - I throw an exception in all cases. If you want to do something specific at each call site, your code will be more complex.

The main piece behind this solution is an IDisposable object, Key which holds a reference to the object we locked on (this allows re-use across multiple classes). If we put the Key in a using statement, it will unlock itself.

public class Key : IDisposable
{
    private object padlock;

    public Key(object locker)
    {
        padlock = locker;
    }

    public void Dispose()
    {
        // when this falls out of scope (after a using {...} ), release the lock
        Monitor.Exit(padlock);
    }
} 

One Way - Less Code

Then add a method to the class to acquire and return a Key.

protected Key Lock()
{
    if (System.Threading.Monitor.TryEnter(this.syncObject, LockTime))
        return new Key(this.syncObject);
    else
        // throw exception, log message, etc.
        throw new System.TimeoutException("(class) failed to acquire the lock.");
} 

With this method, just wrap your work with a single using call.

using (Lock())
{ ... } 

Again, I throw exceptions because it fits the component where I am using this code. The error message could easily be made into a parameter.

Another Way - DIY Failure

If you want to handle failure your way, create a Key manually which still gives you the try, finally and Monitor.Exit calls for free. Your locking code would look like:

if (Monitor.TryEnter(this.syncObject, LockTime))
{
    using (new Key(this.syncObject))
    {
        // do useful work
    }
}
else
{
    // do what you want when it fails
} 

That's a bit cleaner than the original, and most importantly - you can't forget to unlock it.

Note - What to Lock

You should always lock on a private readonly object, ideally one which is kept for the purpose of synchronization. I have seen code examples like this:

lock (myList) // do not do this
{
    myList = new List(); // do not do this either
    myList.AddRange(something);
} 

If the thread is pre-empted and someone now tries to lock on myList, it's a different object - so the locking is ineffective.

Locking on this or on typeof(...) is a practice that is discouraged by MSDN because these are not necessarily safe to use for locks.

History

  • 2014/4/11: Initial version

License

This article, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Share

About the Author

codemunkeh

Scotland Scotland
Hobbyist developer from the UK.
I just make stuff for the benefit of making stuff.

Comments and Discussions

 
SuggestionAn alternative solution [modified] PinmemberHenrik Jonsson13-Apr-14 3:49 
GeneralRe: An alternative solution PinmemberOleg A.Lukin13-Apr-14 19:35 
GeneralRe: An alternative solution PinmemberFatCatProgrammer14-Apr-14 4:11 
AnswerRe: An alternative solution PinmemberHenrik Jonsson15-Apr-14 8:48 
GeneralRe: An alternative solution PinmemberFatCatProgrammer15-Apr-14 10:41 
GeneralMy vote of 5 PinprofessionalVolynsky Alex12-Apr-14 21:41 
QuestionThis is bad advice. PinmemberAlois Kraus12-Apr-14 21:22 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.

| Advertise | Privacy | Mobile
Web01 | 2.8.140926.1 | Last Updated 12 Apr 2014
Article Copyright 2014 by codemunkeh
Everything else Copyright © CodeProject, 1999-2014
Terms of Service
Layout: fixed | fluid