Is this how to write a Thread-Safe IComparable?

Question

I have a simple class and I want to make it thread-safe. The class needs to implement IComparer. I know that implementing the int CompareTo(T other) in a thread-safe way is not straight-forward. It's easy to allow a deadlock if I don't lock in the right way. I have three questions:

Is this code thread-safe? If not, how can I fix it?
Can this code be any shorter? It seems like a lot of code for a simple subtraction.
Should I even bother making int CompareTo(T other) thread-safe? Should I instead require that the caller (usually sorting) lock all of the relevant BObjects?

Here's my code:

public class BObject : IComparable<BObject>
{
    //Each BObject has a unique object id
    private static int _bObjectId = 0;
    private static int GetNextId()
    {
        return System.Threading.Interlocked.Increment(ref BObject._bObjectId);
    }

    private object _locker = new object();
    private readonly int _id = BObject.GetNextId();

    //Some variable
    private int _sales;
    public int Sales
    {
        get
        {
            lock (this._locker)
                return this._sales;
        }
        set
        {
            lock (this._locker)
                this._sales = value;
        }
    }

    public int CompareTo(BObject other)
    {
        int result;

        //Can I simply do "if (this._id == other._id)"
        if (object.ReferenceEquals(this, other))
            result = 0;
        else
        {
            //get the BObject with the lower id
            BObject lower = this._id < other._id ? this : other;

            //get the BObject with the higher id
            BObject higher = this._id > other._id ? this : other;

            //lock the BObject with the lower id first
            lock (lower._locker)
            {
                //lock the BObject with the higher id last
                lock (higher._locker)
                {
                    //put object with lower Sales first
                    result = this.Sales - other.Sales;
                }
            }
        }

        return result;
    }
}

In particular, `int`s are already atomic; you don't need locks at all. — SLaks, Aug 16 '13 at 13:49
Simplest way to make the comparison thread-safe: make your type immutable. — Jon Skeet, Aug 16 '13 at 13:49
`Should I even bother making int CompareTo(T other) thread-safe?` depends on your usage senario. — I4V, Aug 16 '13 at 13:50
And your locks will still encourage you to write unsafe code, such as `someBObject.Sales++`. — SLaks, Aug 16 '13 at 13:50
Change this comment: `lock the BObject with the higher id first` to `...last` — xanatos, Aug 16 '13 at 13:50
For more information, see my blog post, http://blog.slaks.net/2013-07-22/thread-safe-data-structures/ — SLaks, Aug 16 '13 at 13:50
@SLaks: Atomic != volatile. If the OP wants to see the most recently-set value (rather than a potentially stale value) they either need a lock or some other kind of explicit memory barrier (e.g. via Interlocked). — Jon Skeet, Aug 16 '13 at 13:50
@JonSkeet: Making the field `volatile` isn't enough for that? — SLaks, Aug 16 '13 at 13:51
@SLaks: Possibly - volatility scares me. But your initial comment implied (IMO) that *no* other action was required, that if you were using `int` values you didn't need to worry about what other threads were doing at all. — Jon Skeet, Aug 16 '13 at 13:53
@xanatos: That's a good start, but it would end up being very prone to deadlock (and it would still be difficult to ensure that it's called correctly). Immutability is the ideal solution, for a number of reasons. — SLaks, Aug 16 '13 at 13:56
In short, thread-safety is _hard_. You need to figure out what operations can happen concurrently and how to make sure that you are in a consistent state no matter what happens. Immutable objects make this _much_ easier to do. — SLaks, Aug 16 '13 at 13:57
@SLaks No, it's a bad idea. It would need to return an IDisposable object to auto-unlock. Doable but then it becomes difficult. — xanatos, Aug 16 '13 at 13:57
@xanatos: No; it would take a lambda and run it in the lock. Either way, though, you'll be running re-entrant code, which is a recipe for deadlocks. — SLaks, Aug 16 '13 at 14:16
@Slaks I should have used something other than an `int` for an example. If I change `Sales` to be a `string`, then the comparison is not atomic. — user2023861, Aug 16 '13 at 14:24
@user2023861 strings are immutable, so they don't have problems. You simply should have put two fields. int Sales and DateTime LastSale for example — xanatos, Aug 16 '13 at 14:28

score 2 · Answer 1 · answered Aug 16 '13 at 13:54

Under what usage condition do you expect this comparison to occur at the same time as mutations to the value being compared? Under those conditions, what behavior should be 'correct'? Once you define the criteria for correctness, you can design an approach for how to implement that with thread safety.

Thread-safety is really about how things are used and how this usage can interact across thread boundaries. So, for instance, if you're sorting a list of these objects and then mutating the collection at the same time, you might want some way to prevent mutations while in the middle of sorting. Worst case, you could come up with a scenario where you're mutating the instances in a way that causes the sort to never terminate (would be pretty tricky to do, but theoretically possible.) In short, you need to think more about the high-level perspective of how you're using these instances. Most likely, that's not something that can be made 'thread-safe' at the level of instance accessors.

`you might want some way to prevent mutations while in the middle of sorting` This is my thought concerning my third question. In that case it makes no sense to make my `CompareTo` method thread-safe. — user2023861, Aug 16 '13 at 14:20

Is this how to write a Thread-Safe IComparable?

1 Answers1