Need some advice to make the code multithreaded

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开发者_StackOverflow 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

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).

If this is the first time you're doing threading, consider not accessing the singleton from the background thread. You can get it right, but you probably won't get it right the first time.

Realize that if your singleton exposes pointers to other objects, these should be made thread safe as well.

You don't have to define a mutex for each member. For example, you could instead use a single mutex to synchronize access each to member, e.g.:

class foo
{
public:
  ...
  void some_op()
  {
    // acquire "lock_" and release using RAII ...
    Lock(lock_);
    a++;
  }

  void set_b(bar * b)
  {
    // acquire "lock_" and release using RAII ...
    Lock(lock_);
    b_ = b;
  }

private:

  int a_;
  bar * b_;

  mutex lock_;
}

Of course a "one lock" solution may be not suitable in your case. That's up to you to decide. Regardless, simply introducing locks doesn't make the code thread-safe. You have to use them in the right place in the right way to avoid race conditions, deadlocks, etc. There are lots of concurrency issues you could run in to.

Furthermore you don't always need mutexes, or other threading mechanisms like TSS, to make code thread-safe. For example, the following function "func" is thread-safe:

class Foo;
void func (Foo & f)
{
  f.some_op();  // Foo::some_op() of course needs to be thread-safe.
}

// Thread 1
Foo a;
func(&a);

// Thread 2
Foo b;
func(&b);

While the func function above is thread-safe the operations it invokes may not be thread-safe. The point is you don't always need to pepper your code with mutexes and other threading mechanisms to make the code thread safe. Sometimes restructuring the code is sufficient.

There's a lot of literature on multithreaded programming. It's definitely not easy to get right so take your time in understanding the nuances, and take advantage of existing frameworks like Boost.Thread to mitigate some of the inherent and accidental complexities that exist in the lower-level multithreading APIs.

I'd really recommend the Interlocked.... Methods to increment, decrement and CompareAndSwap values when using code that needs to be multi-thread-aware. I don't have 1st-hand C++ experience but a quick search for http://www.bing.com/search?q=c%2B%2B+interlocked reveals lots of confirming advice. If you need perf, these will likely be faster than locking.

As stated by @Void a mutex alone is not always the solution to a concurrency problem:

Regardless, simply introducing locks doesn't make the code thread-safe. You have to use them in the right place in the right way to avoid race conditions, deadlocks, etc. There are lots of concurrency issues you could run in to.

I want to add another example:

class MyClass
{
    mutex m_mutex; 
    AnotherClass m_anotherClass;

    void setObject(AnotherClass& anotherClass)
    {
        m_mutex.lock();
        m_anotherClass = anotherClass;
        m_mutex.unlock();
    }

    AnotherClass getObject()
    {
        AnotherClass anotherClass;

        m_mutex.lock();
        anotherClass = m_anotherClass;
        m_mutex.unlock();

        return anotherClass;
    }
}

In this case the getObject() method is always safe because is protected with mutex and you have a copy of the object which is returned to the caller which may be a different class and thread. This means you are working on a copy which might be old (in the meantime another thread might have changed the m_anotherClass by calling setObject() ).Now what if you turn m_anotherClass to a pointer instead of an object-variable ?

class MyClass
{
    mutex m_mutex; 
    AnotherClass *m_anotherClass;

    void setObject(AnotherClass *anotherClass)
    {
        m_mutex.lock();
        m_anotherClass = anotherClass;
        m_mutex.unlock();
    }

    AnotherClass * getObject()
    {
        AnotherClass *anotherClass;

        m_mutex.lock();
        anotherClass = m_anotherClass;
        m_mutex.unlock();

        return anotherClass;
    }
}

This is an example where a mutex is not enough to solve all the problems. With pointers you can have a copy only of the pointer but the pointed object is the same in the both the caller and the method. So even if the pointer was valid at the time that the getObject() was called you don't have any guarantee that the pointed value will exists during the operation you are performing with it. This is simply because you don't have control on the object lifetime. That's why you should use object-variables as much as possible and avoid pointers (if you can).