Sign in to follow this  
ZergUser

Thread Safety

Recommended Posts

ZergUser    122
The code below is from an article on the singleton pattern and it said, "If you are sure that the first call to instance() will be made in a single-threaded mode, such as during static initialization, then you don't need the lock."
class T
  {
  public:
    static T* instance();
  private:
    T() {}
    ~T() {}
    static T* smInstance;
    static VMutex smMutex;
  };

// T.cpp:

T* T::smInstance = NULL;
VMutex T::smMutex;

T* T::instance()
  {
  VMutexLocker lock(&smMutex);
  
  if (smInstance == NULL)
    smInstance = new T();

  return smInstance;
  }

But without the lock, many threads could potentially be reading smInstance simultaneously. Would this only be a thread safety issue for an SMP machine? Thanks.

Share this post


Link to post
Share on other sites
SiCrane    11839
smInstance can only be written to if smInstance is null. If smInstance is initialized to a non-null value before any additional threads are created, then smInstance can be safely read by multiple threads since no thread will ever try writing to it. (The stuff pointed to by smInstance is another story, however.)

Share this post


Link to post
Share on other sites
fpsgamer    856
Those provisions simply ensure that you don't leak memory through getInstance().

The rest of your implementation will still need to be synchronized appropriately, but that depends on your implementation.

Share this post


Link to post
Share on other sites
Antheus    2409
This is a huge reason why objects that maintain a state shouldn't be singletons - you need to apply synchronization to *every single access* to that singleton, even if you know you're already locked.

Yes, you need to lock access to singleton instance as well. This issue became painfully apparent in Java, where singletons were used for too many types. Not only that, but issues with synchronization (called double-checked locking) were exposed as critical flaws, leading to resource duplication.

Non-const static members with state have no place in multi-threaded applications. There's enough problems with synchronization of transient state as it is.

The only place where singletons can still be used is in purely single-threaded context, and, in case of threads, by providing each thread its own copy of the resource (impossible with pure singletons).

Quote:
such as during static initialization, then you don't need the lock."


Then you don't need a singleton, but a regular static variable.

Singletons are define by being created on first use - that is all that singleton is. Take that away, and you no longer need them.

Share this post


Link to post
Share on other sites
ZergUser    122
Quote:

smInstance can only be written to if smInstance is null. If smInstance is initialized to a non-null value before any additional threads are created, then smInstance can be safely read by multiple threads since no thread will ever try writing to it.


I understand that no other threads will try writing to smInstance because it will be set in a single-threaded mode before any other threads are created. However, in the below line in the instance function, smInstance may be being read from memory by many threads at the same time who want to grab the pointer to the singleton.


if (smInstance == NULL)



Aren't you suppose to synchronize all access to any shared data?

Quote:

(The stuff pointed to by smInstance is another story, however.)


So, I was wondering if the below code would be thread safe. These threads would be created after smInstance is set.


// Thread 1
T* pT = T::instance();

pT->func();

// Thread 2
T* pT = T::instance();

pT->func();



Suppose func doesn't even modify the state of the singleton. It just reads a member variable and returns its value. The above code would still not be thread safe because many threads could be reading the member variable at the same time.

Is this true?

Share this post


Link to post
Share on other sites
Antheus    2409
Quote:
So, I was wondering if the below code would be thread safe. These threads would be created after smInstance is set.


Who said smInstance was set?

If you, then you don't need a singleton. You know the variable has been initialized, so you no longer need a singleton.

In a singleton pattern, you do not know when (if at all) the instance will be set. If you can control the life-cycle, then it's not a singleton anymore.

The above is abuse of abuse of singletons. First, you statically initialize them. Second, you override thread-safety based on faulty assumptions - singletons that refer to other singletons will no longer have "on first use" guarantee, and that happens instantly in Singleton-based design.

In that case, your singleton is better just:

static T t( ... );



You'll only call T members after t has been initialized. No need for getInstance() method.

Talking about initializing the singleton to contain a value is like talking about command pattern being a factory. They are different things - singleton makes no guarantee about existence of its content.

Quote:
The code below is from an article on the singleton pattern and it said, "If you are sure that the first call to instance() will be made in a single-threaded mode, such as during static initialization, then you don't need the lock."


This another flaw - static initialization isn't guaranteed to succeed - you do not have any guarantee by language or anyone else (static initialization order fiasco). Singletons are popular since they eliminate this problem. Removing "on first use" brings you back to original problem.

If you want to avoid locks, then CAS approach should work:

X *getInstance()
{
if (smInstance == NULL) { 1)
X *x = new X();

// if smInstance is null, set it to x
if ( !CAS( smInstance, null, x ) ) { 2)
// smInstance wasn't null,
// someone modified in between 1) and 2),
// so delete local copy
delete x;
}
}
return smInstance;
}

volatile X *smInstance;



Unfortunately, CAS isn't necessarily supported on all hardware.

If you want to make Singleton-based design, you need to make them thread-safe.
If you want to rely on static initialization, don't use singletons at all.

Share this post


Link to post
Share on other sites
ZergUser    122
Quote:
Who said smInstance was set?

If you, then you don't need a singleton. You know the variable has been initialized, so you no longer need a singleton.

In a singleton pattern, you do not know when (if at all) the instance will be set. If you can control the life-cycle, then it's not a singleton anymore.


Suppose T is expensive to create or controls some device, then you would need T to be a singleton. You know for sure that your application is going to use T. So, at some point in time it needs to be created.

Why is T no longer a singleton if you can control the life-cycle?

You still need to guarantee that only one instance of T can be created.

Quote:

In that case, your singleton is better just:

static T t( ... );

You'll only call T members after t has been initialized. No need for getInstance() method.


You could do this but it is no longer a singleton. How can you enforce that only one instance of T is created?

Quote:
This another flaw - static initialization isn't guaranteed to succeed - you do not have any guarantee by language or anyone else (static initialization order fiasco). Singletons are popular since they eliminate this problem. Removing "on first use" brings you back to original problem.


I read about that static initialization order fiasco. T contains two static data members, smInstance and smMutex. The pointer could be statically created without any worries about other static dependencies and I'm assuming the same for smMutex. You could create the singleton as soon as you enter the main function and then spawn off those threads that will eventually grab the singleton instance pointer later.

In general, I was wondering, do you need to synchronize access to a block of memory that is being read simultaneously by many threads to ensure thread safety?

In this case, smInstance is that shared block of memory.


X *getInstance()
{
if (smInstance == NULL) { 1)
X *x = new X();

// if smInstance is null, set it to x
if ( !CAS( smInstance, null, x ) ) { 2)
// smInstance wasn't null,
// someone modified in between 1) and 2),
// so delete local copy
delete x;
}
}
return smInstance;
}

volatile X *smInstance;



Is the above code thread safe, even though smInstance can be read at the same time by many threads?

Why is smInstance volatile?

Thanks, I appreciate the responses.



Share this post


Link to post
Share on other sites
SiCrane    11839
Generally its safe for multiple threads to be reading the same memory as long as no threads are writing.

When you declare a variable as volatile it lets the compiler know that the variable may be altered by outside forces, so when it does computations on the variable it won't cache the value, etc.

Share this post


Link to post
Share on other sites
Antheus    2409
Quote:
In general, I was wondering, do you need to synchronize access to a block of memory that is being read simultaneously by many threads to ensure thread safety?


If the value of the variable can change, then yes. Static initialization means the variable can change.

While it can be avoided in certain cases, it has a subtle flaw. Unless the variable is marked as volatile, on multicore systems the variable may be accessed and cached for different periods of time on different cores. This can happen:

0) x has unknown value
1) create threads
2) x = 17;
3) start threads
4) each thread reads x and displays it



This may look like safe code. After all, x was written in non-threaded environment. Not on multi-core. You have no guarantee that all cores have flushed the old value of x, even though it was set in non-threaded mode.

This is an incredibly rare, yet so subtle flaw that can make debugging nightmare.

Generally, unless you have hardware guarantee, the write itself is *not* required to be atomic. So you could end up with value that has only written first 16 bits of a 32-bit variable.

There are two ways to thread-safety: Thread-safe, and non Thread-safe. There is no half-safe way.

Quote:
I read about that static initialization order fiasco. T contains two static data members, smInstance and smMutex.


They don't need to be static. They just need to be global, defined in different units.

Quote:
Is the above code thread safe, even though smInstance can be read at the same time by many threads?


It's safe, if smInstance is initialized to NULL before first access. C++ doesn't do that by itself.

Quote:
Why is smInstance volatile?


See above, for multi-core issues with variable caching.

Quote:
Why is T no longer a singleton if you can control the life-cycle?


Because then it becomes wrapper to static variable. Singleton is defined by the very fact that it creates itself when first called.

Quote:
You still need to guarantee that only one instance of T can be created.


This has nothing to do with singletons. Singletons ensure that you always get the same instance, not that you cannot have more than one (this case doesn't happen as a consequence of previous rule, but it doesn't prevent you from spawning multiple instances of underlying object). Not making getInstance() thread-safe you no longer have that guarantee.

Quote:
You could do this but it is no longer a singleton. How can you enforce that only one instance of T is created?


The same way you want to initialize the singleton manually to make sure it's valid (by coding carefully). You can prevent multiple instances by declaring T as non-copyable (private assignment operator).


Singletons are really really annoying to do correctly, yet very easy to get working in 95% of the cases (or some 50% of the cases in multi-core systems).

Share this post


Link to post
Share on other sites
ZergUser    122
Quote:
While it can be avoided in certain cases, it has a subtle flaw. Unless the variable is marked as volatile, on multicore systems the variable may be accessed and cached for different periods of time on different cores. This can happen:

0) x has unknown value
1) create threads
2) x = 17;
3) start threads
4) each thread reads x and displays it

This may look like safe code. After all, x was written in non-threaded environment. Not on multi-core. You have no guarantee that all cores have flushed the old value of x, even though it was set in non-threaded mode.

This is an incredibly rare, yet so subtle flaw that can make debugging nightmare.


So, the volatile keyword ensures that x gets read from main memory all the time.
This would remove any issues with the compiler caching the value in a register.

However even if x is qualified with volatile, could out-of-order execution cause each thread to display different values(initial garbage value or 17)?

Quote:

Quote:
Why is T no longer a singleton if you can control the life-cycle?

Because then it becomes wrapper to static variable. Singleton is defined by the very fact that it creates itself when first called.


This makes sense now. I guess you really don't need that comparison for 0 if you know it will not be 0.

Quote:

Quote:
You still need to guarantee that only one instance of T can be created.

This has nothing to do with singletons. Singletons ensure that you always get the same instance, not that you cannot have more than one (this case doesn't happen as a consequence of previous rule, but it doesn't prevent you from spawning multiple instances of underlying object). Not making getInstance() thread-safe you no longer have that guarantee.


My design patterns book says the intent of the singleton is to ensure that a class only has one instance, and provide a global point of access to it.
This makes sense if the object is very expensive to create.

Share this post


Link to post
Share on other sites
Antheus    2409
Quote:
However even if x is qualified with volatile, could out-of-order execution cause each thread to display different values(initial garbage value or 17)?


If volatile, compiler must not make any kind of optimization to the variable whatsoever, not even re-arrange the code. This incurs a severe performance hit, but makes accessing volatile variables safer.

Quote:
My design patterns book says the intent of the singleton is to ensure that a class only has one instance, and provide a global point of access to it.


Singletons and multi-threading don't mix. And getInstance is the least of your problems. If you have a singleton, you have shared resource. And if this resource is on critical path, you have a single-threaded application.

If you're writing a multi-threaded application, you duplicate resources as much as possible across threads and always try to remove any kind of congestion through locks (not by blindly removing them, but by changing the design that uses these resources).

You have a conflict of interests here. Singletons are great for single-threaded applications. Messing with that tiny getInstance penalty won't do you any good in the long run - the performance hit will come since on every access to the singleton object you'll need to synchronize access. The penalty of getInstance lookup is zero - the cost comes from contention for the singleton. If there is no contention, then it doesn't need to be a singleton in the first place, since you can create it before you need it.

Singleton was one of the original design patterns. It took about a year for entire software industry to agree that it's a complete disaster even in more complex languages, where you don't deal with all the issues C++ has. Java VM cheated on synchronization, and the end result was broken singleton implementation which resulted in broken singletons until atomic operations were added. But even before then, they were obsoleted.

Quote:
This makes sense if the object is very expensive to create.


Wrong in case of multi-threading. One-time creation of the object is cheap, as long as duplicating it prevents any kind of congestion.

RealWorld (tm) example - multi-threaded zip stream compression (network server, client stream compression). Each zlib state consumes ~256kb of memory, and takes long time to initialize (several ms - can't create them on the fly). Singleton? Nope. One per thread. Reason is simple - what good is multi-threaded decompressor, when its internal state can only be accessed once at a time? And yes.... I made it a global object at first, to save memory...


Just so sum up all the above.

Multi-threading is hard. Not a little, but insanely hard. You can run across bugs once every 3 months that you'll never be able to fix. As such, cheating on such little things is a really bad way to optimize things.

In multi-threading, you either establish very strict communication paths (who knows about what, who manages what, who talks to whom), duplicate resources (each thread has its own game engine, or the world objects are split between threads, or engine, physics and IO each run in their own thread). But singleton is the anti-thesis of concurrency. It's a shared, contested global state.

If it makes sense to share it, it will bog your entire application down to performance barely better than that of single-threaded (or worse). The only solution is not to use them. Or if you do - make them stateless, or make them have immutable state (one that cannot change once they have been created). That allows you to access them without penalties.

Otherwise, duplicate the resources, or distribute them among threads.

Perhaps the most typical problems you'll run into with static members and multi-threading are pooled memory allocators. If static, you need to synchronize them, killing performance, or use lockless approach similar to that above.

Share this post


Link to post
Share on other sites
ZergUser    122
Quote:
Quote:
However even if x is qualified with volatile, could out-of-order execution cause each thread to display different values(initial garbage value or 17)?
If volatile, compiler must not make any kind of optimization to the variable whatsoever, not even re-arrange the code. This incurs a severe performance hit, but makes accessing volatile variables safer.


From Wikipedia:
In C, the volatile keyword is provided to inhibit optimisations which remove or reorder memory operations on a variable marked as volatile. This will provide a kind of barrier for interruptions which occur on a single CPU, such as signal handlers or concurrent threads on a uniprocessor system. However, the use of volatile is insufficient to guarantee correct ordering for multiprocessor systems because it only impacts reorderings performed by the compiler, not those which may occur at runtime such as those performed by the CPU.

So, you really would need a memory barrier after x=17 for everything to work as expected on a multi-core machine?

Thanks for the wrap-up on singleton. I need to try to use more duplication and write reentrant functions. I think it would be much easier to have separate code streams in their own address spaces and rely on the OS for the communication. But, you pay for it in performance.

Share this post


Link to post
Share on other sites
Antheus    2409
Quote:
the volatile keyword is provided to inhibit optimisations which remove or reorder memory operations on a variable marked


Not necessarily.

This is where it gets ugly - it depends on language, OS, compiler and hardware. And bugs.

And C isn't the best example. MVS 2005 (obviously relying on Windows API) handles just about all of the additional problems, including barriers. But I still won't guarantee that it's truly safe, I simply don't have enough experience with this.

Once again - concurrent programming is hard. So never make assumptions - use that extra lock, or change the design.

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this