Need some advice to make the code multithreaded

Question

I received a code that is not for multi-threaded app, now I have to modify the code to support for multi-threaded.

I have a Singleton class(MyCenterSigltonClass) that based on instruction in: http://en.wikipedia.org/wiki/Singleton_pattern I made it thread-safe

Now I see inside the class that contains 10-12 members, some with getter/setter methods. Some members are declared as static and are class pointer like:

static Class_A*    f_static_member_a;
static Class_B*    f_static_member_b;

for these members, I defined a mutex(like mutex_a) INSIDE the class(Class_A) , I didn't add the mutex directly in my MyCenterSigltonClass, the reason is they are one to one association with my MyCenterSigltonClass, I think I have option to define mutex in the class(MyCenterSigltonClass) or (Class_A) for f_static_member_a.

1) Am I right?

Also, my Singleton class(MyCenterSigltonClass) contains some other members like

Class_C  f_classC;

for these kind of member variables, should I define a mutex for each of them in MyCenterSigltonClass to make them thread-safe? what would be a good way to handle these cases?

Appreciate for any suggestion.

-Nima

Answer 1

Whether the members are static or not doesn't really matter. How you protect the member variables really depends on how they are accessed from public methods.

You should think about a mutex as a lock that protects some resource from concurrent read/write access. You don't need to think about protecting the internal class objects necessarily, but the resources within them. You also need to consider the scope of the locks you'll be using, especially if the code wasn't originally designed to be multithreaded. Let me give a few simple examples.

class A
{
private:
    int mValuesCount;
    int* mValues;

public:
    A(int count, int* values)
    {
        mValuesCount = count;
        mValues = (count > 0) ? new int[count] : NULL;
        if (mValues)
        {
            memcpy(mValues, values, count * sizeof(int));
        }
    }

    int getValues(int count, int* values) const
    {
        if (mValues && values)
        {
            memcpy(values, mValues, (count < mValuesCount) ? count : mValuesCount);
        }
        return mValuesCount; 
    }
};

class B
{
private:
    A* mA;

public:
    B()
    {
        int values[5] = { 1, 2, 3, 4, 5 };
        mA = new A(5, values);
    }
    const A* getA() const { return mA; }
};

In this code, there's no need to protect mA because there's no chance of conflicting access across multiple threads. None of the threads can modify the state of mA, so all concurrent access just reads from mA. However, if we modify class A:

class A
{
private:
    int mValuesCount;
    int* mValues;

public:
    A(int count, int* values)
    {
        mValuesCount = 0;
        mValues = NULL;
        setValues(count, values);
    }

    int getValues(int count, int* values) const
    {
        if (mValues && values)
        {
            memcpy(values, mValues, (count < mValuesCount) ? count : mValuesCount);
        }
        return mValuesCount; 
    }

    void setValues(int count, int* values)
    {
        delete [] mValues;

        mValuesCount = count;
        mValues = (count > 0) ? new int[count] : NULL;
        if (mValues)
        {
            memcpy(mValues, values, count * sizeof(int));
        }
    }
};

We can now have multiple threads calling B::getA() and one thread can read from mA while another thread writes to mA. Consider the following thread interaction:

Thread A: a->getValues(maxCount, values);
Thread B: a->setValues(newCount, newValues);

It's possible that Thread B will delete mValues while Thread A is in the middle of copying it. In this case, you would need a mutex within class A to protect access to mValues and mValuesCount:

    int getValues(int count, int* values) const
    {
        // TODO: Lock mutex.
        if (mValues && values)
        {
            memcpy(values, mValues, (count < mValuesCount) ? count : mValuesCount);
        }
        int returnCount = mValuesCount;
        // TODO: Unlock mutex.
        return returnCount; 
    }

    void setValues(int count, int* values)
    {
        // TODO: Lock mutex.
        delete [] mValues;

        mValuesCount = count;
        mValues = (count > 0) ? new int[count] : NULL;
        if (mValues)
        {
            memcpy(mValues, values, count * sizeof(int));
        }
        // TODO: Unlock mutex.
    }

This will prevent concurrent read/write on mValues and mValuesCount. Depending on the locking mechanisms available in your environment, you may be able to use a read-only locking mechanism in getValues() to prevent multiple threads from blocking on concurrent read access.

However, you'll also need to understand the scope of the locking you need to implement if class A is more complex:

class A
{
private:
    int mValuesCount;
    int* mValues;

public:
    A(int count, int* values)
    {
        mValuesCount = 0;
        mValues = NULL;
        setValues(count, values);
    }

    int getValueCount() const { return mValuesCount; }

    int getValues(int count, int* values) const
    {
        if (mValues && values)
        {
            memcpy(values, mValues, (count < mValuesCount) ? count : mValuesCount);
        }
        return mValuesCount; 
    }

    void setValues(int count, int* values)
    {
        delete [] mValues;

        mValuesCount = count;
        mValues = (count > 0) ? new int[count] : NULL;
        if (mValues)
        {
            memcpy(mValues, values, count * sizeof(int));
        }
    }
};

In this case, you could have the following thread interaction:

Thread A: int maxCount = a->getValueCount();
Thread A: // allocate memory for "maxCount" int values
Thread B: a->setValues(newCount, newValues);
Thread A: a->getValues(maxCount, values);

Thread A has been written as though calls to getValueCount() and getValues() will be an uninterrupted operation, but Thread B has potentially changed the count in the middle of Thread A's operations. Depending on whether the new count is larger or smaller than the original count, it may take a while before you discover this problem. In this case, class A would need to be redesigned or it would need to provide some kind of transaction support so the thread using class A could block/unblock other threads:

Thread A: a->lockValues();
Thread A: int maxCount = a->getValueCount();
Thread A: // allocate memory for "maxCount" int values
Thread B: a->setValues(newCount, newValues); // Blocks until Thread A calls unlockValues()
Thread A: a->getValues(maxCount, values);
Thread A: a->unlockValues();
Thread B: // Completes call to setValues()

Since the code wasn't initially designed to be multithreaded, it's very likely you'll run into these kinds of issues where one method call uses information from an earlier call, but there was never a concern for the state of the object changing between those calls.

Now, begin to imagine what could happen if there are complex state dependencies among the objects within your singleton and multiple threads can modify the state of those internal objects. It can all become very, very messy with a large number of threads and debugging can become very difficult.

So as you try to make your singleton thread-safe, you need to look at several layers of object interactions. Some good questions to ask:

1. Do any of the methods on the singleton reveal internal state that may change between method calls (as in the last example I mention)?
2. Are any of the internal objects revealed to clients of the singleton?
3. If so, do any of the methods on those internal objects reveal internal state that may change between method calls?
4. If internal objects are revealed, do they share any resources or state dependencies?

You may not need any locking if you're just reading state from internal objects (first example). You may need to provide simple locking to prevent concurrent read/write access (second example). You may need to redesign the classes or provide clients with the ability to lock object state (third example). Or you may need to implement more complex locking where internal objects share state information across threads (e.g. a lock on a resource in class Foo requires a lock on a resource in class Bar, but locking that resource in class Bar doesn't necessarily require a lock on a resource in class Foo).

Implementing thread-safe code can become a complex task depending on how all your objects interact. It can be much more complicated than the examples I've given here. Just be sure you clearly understand how your classes are used and how they interact (and be prepared to spend some time tracking down difficult to reproduce bugs).