Singleton getInstance in thread worker methods

Question

Hello,

This question is about using getter methods of a singleton object in worker threads. Here is some pseudo code first:

// Singleton class which contains data
class MyData
{
    static MyData* sMyData ;

    int  mData1[1024];
    int  mData2[1024];
    int  mData3[1024];

    MyData* getInstance()
    {
        // sMyData is created in the very beginning.
        return sMyData ;
    }

    void getValues(int idx, int& data1,int& data2,int& data3)
    {
        data1 = mData1[idx];
        data2 = mData2[idx];
        data3 = mData3[idx];
    }

    int* getData1()
    {
        return &mData1[0];
    }
}

class MyThread
{
    void workerMethod()
    {
        MyData* md = MyData::getInstance();

        int d1,d2,d3;
        md->getValue( 12, d1,d2,d3 );

        int* data1 = md->getData1();
        d1 = data1[34];
    }
}

Now as you see I have some getter methods (all read-only), MyData::getInstance(), MyData::getValue() and MyData::getData1(). The 1st question is how thread-safe these methods are ?

Since they are often-called methods, protecting those methods with mutex is something I am trying to avoid.

The 2nd question is: what is the suggested way of reading data from central sources in a multi-thread application, especially in worker methods.

Thanks !

Paul

Answer 1

Provided that no other thread will try to write to the data in your singleton object, you don't need to protect them: by definition, multiple readers in the absence of a writer is thread-safe. This is a common pattern where the program's initialization code sets up a singleton, which is then only read from by worker threads.

However, if any thread ever writes to this data while others are reading from it, you must protect it in some way. If you have lots of readers and only the occasional writer, it is worth considering some sort of "read-write" lock, which allows multiple readers in the absence of any writers.

Answer 2

Your methods are okay, but you have two major problems.

First, MyData::getInstance() should be static and does not create an instance. In order to minimize mutex usage, you should check out the double checked lock.

Second, thread safety is up to callers of the getter methods not the MyData class. If that is what you're looking for, then great. If not, then you are going to have to come up with some sort of access control in MyClass. Also, since your contained data type is just an basic type (int), chances are good that you'll never see any synchronization problems until after your code is in production.

Answer 3

It is not possible to tell if this could is threadsafe. If the data is initialized during object creation and never changes than this will run correctly. If you are mutating the underlying data through other methods then the readers will have to perform some sort of synchronization against the writers, there is no way around that.

Depending on exactly what you are doing you might be able to use something lighter weight than a mutex, such as atomic updates synchronize commands, or using reader-writer locks, but without knowing more about what you are doing it is impossible to tell.

Answer 4

First go back and read this question to get a better version of singelton:
http://stackoverflow.com/questions/270947/can-any-one-provide-me-a-sample-of-singleton-in-c

Also note: Do NOT use a singleton as a glorified global variable.
It just adds complexity to a bad design. Just use a global variable.

As long as you are only reading from the singleton it is thread safe when being used.

The only point that is not thread safe (not guaranteed by the language) is during creation. So technically you should add locks around the part that creates the instance to guarantee that singleton has been fully created before anybody can use it.

Note: Do not be lured by the use of double checked locking to optimize your locking strategy. It can NOT be made to work correctly in C++. Read this DDJ article.

If you can guarantee that the singleton is instantiated (probably the first call too getInstance()) within a single threaded environment (before any threads are created) then you can dispense with the need for locks during instantiation.

If you alter your code so that other threads can write to the singleton then you need to think about locking and consistency. You only need locking if your writes are not atomic. If your write is just updating one integer it is already atomic no change required. If your code is not atomic:

• Writing multiple integers within a method
• Performing a read and then write

Then you will need to lock the object until all writes are complete and consequently also have locks on the methods that have read access to stop them reading from the object while another method updates the object.

Consequently this is another reason why member variables should only be accessed by member methods.

Answer 5

For thread safety you need to look at the class as a whole. As written your class would not be thread safe. Whilst your getValues method is okay, the getData1 method has problems.

You say they are (read-only) getter methods. However, neither are declared as const methods. The getData1 would not be valid as a const method as it returns a non const pointer. In addition, returning a pointer to private class data is bad as you are exposing your implementation.

If this is a singleton class to hold some essentially static data set on initialistion before your threading kicks off then all your accessors should be const methods. The getInstance should also return a const pointer to the class (and be a static method as mentioned by another answer).