6
\$\begingroup\$

I recently found out that a class library I have been building, xofz.Core, could mostly be salvaged and back-ported to .NET Framework 2.0. This enables it to be used to build applications for Windows 98! How cool is that?

Here is a link to the repository: https://github.com/spearson/xofz.Core98

And the class I would like looked over most now is the Tuple implementation. It started out as a clone of the Tuple class from .NET 4 and on, but then just got AtomicGet() and AtomicSet() methods. What do you think of this class, and in particular those methods? I'm not precisely sure, but I imagine they could have a real use down the line.

Here are the tuple classes:

namespace xofz
{
    using System.Threading;

    public class Tuple<T, U>
    {
        public Tuple(T item1, U item2)
        {
            this.Item1 = item1;
            this.Item2 = item2;
        }

        public virtual T Item1 { get; private set; }

        public virtual U Item2 { get; private set; }

        public virtual void AtomicGet(out T item1, out U item2)
        {
            while (Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item2 = this.Item2;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(T item1, U item2)
        {
            while (Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item2 = item2;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        private int settingOrGettingIf1;
    }

    public class Tuple<T, U, V>
    {
        public Tuple(
            T item1,
            U item2,
            V item3)
        {
            this.Item1 = item1;
            this.Item2 = item2;
            this.Item3 = item3;
        }

        public virtual T Item1 { get; private set; }

        public virtual U Item2 { get; private set; }

        public virtual V Item3 { get; private set; }

        public virtual void AtomicGet(out T item1, out U item2)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item2 = this.Item2;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(T item1, U item2)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item2 = item2;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out T item1, out V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item3 = this.Item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(T item1, V item3)
        {
            while (Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item3 = item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out U item2, out V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item2 = this.Item2;
            item3 = this.Item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(U item2, V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item2 = item2;
            this.Item3 = item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out T item1, out U item2, out V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item2 = this.Item2;
            item3 = this.Item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(T item1, U item2, V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item2 = item2;
            this.Item3 = item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        private int settingOrGettingIf1;
    }

    public class Tuple<T, U, V, W>
    {
        public Tuple(
            T item1,
            U item2,
            V item3,
            W item4)
        {
            this.Item1 = item1;
            this.Item2 = item2;
            this.Item3 = item3;
            this.Item4 = item4;
        }

        public virtual T Item1 { get; private set; }

        public virtual U Item2 { get; private set; }

        public virtual V Item3 { get; private set; }

        public virtual W Item4 { get; private set; }

        public virtual void AtomicGet(out T item1, out U item2)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item2 = this.Item2;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(T item1, U item2)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item2 = item2;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out T item1, out V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item3 = this.Item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(T item1, V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item3 = item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out T item1, out W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item4 = this.Item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(T item1, W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item4 = item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out U item2, out V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item2 = this.Item2;
            item3 = this.Item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(U item2, V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item2 = item2;
            this.Item3 = item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out U item2, out W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item2 = this.Item2;
            item4 = this.Item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(U item2, W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item2 = item2;
            this.Item4 = item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out V item3, out W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item3 = this.Item3;
            item4 = this.Item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(V item3, W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item3 = item3;
            this.Item4 = item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out T item1, out U item2, out V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item2 = this.Item2;
            item3 = this.Item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(T item1, U item2, V item3)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item2 = item2;
            this.Item3 = item3;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out T item1, out U item2, out W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item2 = this.Item2;
            item4 = this.Item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(T item1, U item2, W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item2 = item2;
            this.Item4 = item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out T item1, out V item3, out W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item3 = this.Item3;
            item4 = this.Item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(T item1, V item3, W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item3 = item3;
            this.Item4 = item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(out U item2, out V item3, out W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item2 = this.Item2;
            item3 = this.Item3;
            item4 = this.Item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(U item2, V item3, W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item2 = item2;
            this.Item3 = item3;
            this.Item4 = item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicGet(
            out T item1, 
            out U item2, 
            out V item3, 
            out W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            item1 = this.Item1;
            item2 = this.Item2;
            item3 = this.Item3;
            item4 = this.Item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        public virtual void AtomicSet(
            T item1, 
            U item2, 
            V item3, 
            W item4)
        {
            while (Interlocked.CompareExchange(
                       ref this.settingOrGettingIf1, 1, 0) == 1)
            {
                continue;
            }

            this.Item1 = item1;
            this.Item2 = item2;
            this.Item3 = item3;
            this.Item4 = item4;
            Interlocked.CompareExchange(
                ref this.settingOrGettingIf1, 0, 1);
        }

        private int settingOrGettingIf1;
    }

    public static class Tuple
    {
        public static Tuple<T, U> Create<T, U>(
            T item1,
            U item2)
        {
            return new Tuple<T, U>(
                item1,
                item2);
        }


        public static Tuple<T, U, V> Create<T, U, V>(
            T item1,
            U item2,
            V item3)
        {
            return new Tuple<T, U, V>(
                item1,
                item2,
                item3);
        }

        public static Tuple<T, U, V, W> Create<T, U, V, W>(
            T item1,
            U item2,
            V item3,
            W item4)
        {
            return new Tuple<T, U, V, W>(
                item1,
                item2,
                item3,
                item4);
        }
    }
}

On a side note, I hope this class benefits someone else and maybe other items in the class library, as well.

\$\endgroup\$
  • 6
    \$\begingroup\$ Why the use of virtual? Can't think of any good reason why I would subclass a tuple. \$\endgroup\$ – D. Jurcau Aug 12 '17 at 8:40
  • 3
    \$\begingroup\$ Could explain the purpose of the Interlocked parts and the helper variable? \$\endgroup\$ – t3chb0t Aug 12 '17 at 10:12
  • \$\begingroup\$ Thanks, everyone, for your replies! Virtual is there to make tuples super easy to test -- though I can definitely see an argument for simply using real Tuple instances in your tests as well. The Interlocked.CompareExchange parts helps keep reading and writing atomic - if you read, you read the last values that were actually put in the Tuple, not some mix of values based on what was written to and by which thread. It's the only argument for mutability of tuples I could make. \$\endgroup\$ – xofz Aug 12 '17 at 19:47
  • 1
    \$\begingroup\$ @SamPearson: Why do you want to make the tuple mutable? \$\endgroup\$ – JanDotNet Aug 12 '17 at 20:28
5
\$\begingroup\$

I'm not going to comment about the lock-free implementation, as that's not my field of expertise, but here's what I've got:

  • The standard tuple class is immutable. Making it mutable is a rather fundamental change, and quite a 'surprising' design decision. I really wouldn't do this unless I had some very compelling reasons. Especially in concurrency, where shared mutable state is a big source of trouble.
  • If you do have good reasons for doing this, then I'd expect them to be documented somewhere. I'd also expect the different nature of this tuple class to be mentioned - preferrably in an obvious place like the class name itself (MutableTuple).
  • If you don't have a particular reason for doing this (which seems to be the case: "I'm not precisely sure, but I imagine they could have a real use down the line."), then I'd stick to YAGNI: you ain't gonna need it. Chances are that when you need something like this, your needs are slightly different from what this class provides anyway.
  • Instead of T, U, V and W, it's more common (and readable) to use meaningful names with a T prefix: TKey, TValue, or, in the case of tuples, T1, T2, and so on.
  • The AtomicGet and AtomicSet overloads rely on the type parameters being different. If more than two are the same, such as in Tuple<int, int, int>, then code that tries to call the 'partial' get/set methods fails to compile due to an ambiguity error. A simple solution would be to remove those partial overloads - which also reduces the amount of code you need to maintain.
  • You may also want to remove the public property getters. This forces calling code to take the mutable and concurrent nature of your tuple class into account, making it harder to use incorrectly.
\$\endgroup\$
  • 2
    \$\begingroup\$ +1 excellent point about the partial overload method resolution (can be solved with more explicit naming). As a point of information, the new C# 7 tuples are mutable (ValueTuple), though I agree that a different name would be sensible. \$\endgroup\$ – VisualMelon Aug 12 '17 at 16:47
  • \$\begingroup\$ @VisualMelon: I didn't know ValueTuple so far... Indeed, it is mutable. However, It should be mentioned that it is a struct (value type) which behaves different than mutable classes. \$\endgroup\$ – JanDotNet Aug 12 '17 at 20:50
5
\$\begingroup\$

Style

External API looks fine, would appreciate some inline-documentation (\\\) describing the mutability and thread-safety of the class and it's members.

settingOrGettingIf1

While descriptive, this name is not entirely clear, and introduces magic numbers into the code (it says nothing about 0, for example). It would be better, I think, to have a name more along the lines of "LockStatus", and define local, static readonly (or const) variables called "Available" and "Locked" (or whatever makes sense to you) rather than 0 and 1.

Atomic Accessors

The Interlocked/CAS (Compare and Set) mutual exclusion code looks OK. I've not checked every method, I'm assuming they all look the same, but it would be better to pull out private "Grab" and "Release" methods (name them however you like) which will tidy up the atomic methods, and make the code more maintainable, and much easier to read, write, and extend.

I note that you seem to have gone to a lot of work to provide many versions of AtomicGet and AtomicSet for various combinations of Items. However, you have no such method for individual Items (I don't know if you are aware, but you can rarely assume that a simple lookup or assignment will be atomic, certainly can't with generic types). This means that anyone getting Item1 directly could read a 'broken' value (e.g. if T is double, it is possible on a 32bit machine for one thread to write the first 4bytes and another thread to read them before the next 4bytes are written). I would suggest inline documentation (\\\) explaining that these are not atomic and providing alternative AtomicGet methods, or otherwise making them thread-safe (just documenting it seems fine, because it can only cause issues for the caller which is responsible for its own usage, and can't interfere with other (correct) usage).

It should be noted that if you provide any more complicated 'atomic' methods, then it is important to consider how they might fail (e.g. throw exception, more in the virtual section), as unlike a lock statement, this technique can easily go wrong (it also isn't re-entrant, which you'll need to think about if you make the properties themselves atomic (you'll need to separate out the backing field (or use an additional private property) so you can directly access it from the multi-item accessory)).

virtual

As D. Jurcau said in his comment, it is not apparent why you'd want to extend the methods (or properties!) in this class. A Tuple is a simple, generic, reusable data-structure, it shouldn't be overridden to provide business logic, which should express intent through meaningful names and such (Item1 would be a terrible name for most anything but a generic Tuple class!).

One particular way this could cause a headache would be if one of the getters of a property was overridden, it might throw an exception (you can't be sure it won't). If such an exception was thrown while attempting an AtomicGet, it would never release the lock. Such a possibility represents a serious design decision (i.e. how to deal with failures), and such a generic, reusable class shouldn't have to make such decisions (it should be simple and robust). For this reason alone I would absolutely make the properties non-virtual.

You may also get better performance with non-virtual methods, but this is incidental.

Example

Some example code showing what I might change:

// not virtual
/// <summary> Non-atomic, non-thread-safe accessors </summary>
public T Item1 { get; private set; }
/// <summary> Non-atomic, non-thread-safe accessors </summary>
public U Item2 { get; private set; }

private int lockStatus = LockAvailable;

private const int LockAvailable = 0;
private const int LockTaken = 1;

private void GrabLock()
{
    while (Interlocked.CompareExchange(ref this.lockStatus, LockTaken, LockAvailable) != LockAvailable)
    {
        continue;
    }
}

private void ReleaseLock()
{
    Interlocked.CompareExchange(ref this.lockStatus, LockAvailable, LockTaken);
}

// not virtual
// (more inline documentation here)
public void AtomicGet(out T item1, out U item2)
{
    GrabLock(); // short and sweet

    item1 = this.Item1;
    item2 = this.Item2;

    ReleaseLock();
}

A word about testing

Testing threaded code is really hard, and CAS is easy to get wrong. It is important to test such code thoroughly (I'm afraid I don't have any good tips for how to do that), and to document the expected behaviour so that you know what the code is meant to do when you test it, so that the user knows what it is meant to do when they use it, and so that the maintainer knows what it is meant to do when they try to change it.

\$\endgroup\$
  • 1
    \$\begingroup\$ I guess you're right and OP might know this acronym already but I (and perhaps other people) learn from it too - it was new to me - thus the question ;-) \$\endgroup\$ – t3chb0t Aug 12 '17 at 12:56
  • \$\begingroup\$ @t3chb0t you're quite right, thanks for pointing it out ;) \$\endgroup\$ – VisualMelon Aug 12 '17 at 13:01
5
\$\begingroup\$
  • Your Tuple misses the implementation of Equals and GetHashCode.

  • As already mentioned, it absolutely makes sense, that tuples are immutable. That makes the atomic setter/getter unnecessary.

  • The virtual keyword should be dropped. At least, I can't see any reasonable use case for making any members virtual.

\$\endgroup\$
  • \$\begingroup\$ +1: good points about Equals and GetHashCode. Now that I'm looking at the reference source, there are a few other things missing as well: ToString and IComparable (and also IStructuralEquatable and IStructuralComparable, but those didn't exist in .NET 2.0). \$\endgroup\$ – Pieter Witvoet Aug 12 '17 at 19:31

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.