Singleton with template wrapper

Question

So far, I've used this in a few places and it seems solid.

This is what I usually do to make a singleton:

// Assume x86 for now
#ifdef _MSC_VER
#include <intrin.h> // for _mm_pause
#else
#include <xmmintrin.h> // for _mm_pause
#endif
#include <cstdint>
#include <exception>
#include <atomic>
#include <memory>
// When thrown by get(), this indicates the singleton failed to initialize.
//
// It is better to devise a more elaborate set of exceptions for diagnostic purposes 
// (e.g. If it must be initialized in the main thread only, etc...).
class bad_singleton : public std::exception{};
template<typename singleton_t>
class singleton
{
 // Should start life as false, no initialization logic needed
 // (unless working with a brain-dead compiler, in which case
 // we're screwed anyway).
 static bool _bad_singleton;
 // This should also be 0 on start, but some platforms might require
 // a constructor that could fudge things if it runs while the lock is
 // being used (meaning that the platform requires kernel objects for all atomic
 // operations). To make this robust, use a plain uintptr_t here and
 // OS-provided atomic functions (e.g. _InterlockedExchange[64]), If atoms aren't
 // supported, you shouldn't be using threads anyway.
 static std::atomic<uintptr_t> _spinlock;
 // Once again, shared_ptr might require initialization logic
 // that could run after the allocation in get() [assuming we have things
 // running before main]. To make this robust, use a pointer here
 // and OS-provided atomic functions.
 static std::shared_ptr<singleton_t> _handle;
public:
 static std::shared_ptr<singleton_t> get()
 {
 // Assumes acquire semantics on read.
 if(_handle){
 // _handle is nonzero? Instant return!
 return _handle;
 }else{
 // Every thread that found _ptr to be null will end up here
 // and spin until the lock is released.
 while(_spinlock.exchange(1))
 {
 // One can use system calls here (like Sleep(0) on Windows) if desired
 //
 // Otherwise, this has a similar effect without dragging in too much
 // platform-specific gunk.
 _mm_pause();
 }
 // Only one thread at a time here, so check once more and return
 // (another thread could have finished constructing the instance).
 if(_handle){
 _spinlock.exchange(0);
 return _handle;
 }
 if(_bad_singleton){
 // The singleton failed to initialize in another thread.
 // EDIT: Forgot to release the lock
 _spinlock.exchange(0);
 throw bad_singleton();
 }
 // Since it is assumed that the constructor does something
 // simple or nothing at all, the only overhead here is
 // having to allocate space on the heap. This is also
 // why singletons like this should be made initially
 // as small as possible in order to reduce the probability
 // of an allocation failure.
 singleton_t *_frob = nullptr;
 try{
 _frob = new singleton_t();
 // EDIT: Prevent possible premature _handle initialization
 // forces new singleton_t() to finish execution before
 // initializing _handle.
 //
 // It is also assumed that the singleton's constructor
 // returns only when the instance is fully initialized.
 _WriteBarrier(); //<- or equivalent
 std::shared_ptr<singleton_t> _derp(_frob);
 // EDIT: Shared_ptr allocates a control block, so
 // we could still have a garbage pointer if directly
 // constructed into _handle.
 _WriteBarrier();
 // This should not do anything that throws an exception.
 //
 // In fact, this should be atomic (It can be made so in a custom
 // handle implementation).
 _handle = move(_derp);
 }catch(...){
 // Diaper has been soiled...
 // EDIT: Forgot that shared_ptr allocates a control block.
 //
 // Is possible to have _frob without _handle...
 if(_frob)delete _frob;
 // There is probably a way to reproduce this exception
 // for all other threads, but for now, just throw a
 // bad_singleton exception everywhere else.
 _bad_singleton = true;
 _spinlock.exchange(0);
 // Throw whatever was caught for logging.
 throw;
 }
 // Now release the spinlock to let any contending threads in.
 _spinlock.exchange(0);
 return _handle;
 }
 }
};
template<typename singleton_t>
bool singleton<singleton_t>::_bad_singleton;
template<typename singleton_t>
std::atomic<uintptr_t> singleton<singleton_t>::_spinlock;
template<typename singleton_t>
std::shared_ptr<singleton_t> singleton<singleton_t>::_handle;
class my_singleton
{
protected:
 // allows the singleton wrapper to use the constructor
 friend class singleton<my_singleton>;
 my_singleton()
 {
 // Very simple things in here
 //
 // Split singletons into related subsystems
 // that can be initialized when required,
 // but in a controlled way through the instance
 // itself (e.g. allocate a kernel object and
 // use that for serializing initializations of other
 // systems that take longer or are more complex).
 }
private:
 // No duplicating or moving this singleton whatsoever
 my_singleton &operator=(const my_singleton &);
 my_singleton &operator=(my_singleton &&);
 my_singleton(const my_singleton &);
 my_singleton(my_singleton &&);
public:
 // Destructor is out here in the public because shared_ptr needs it.
 //
 // A custom smart handle implementation could allow for private destructors.
 ~my_singleton()
 {
 // Note that this will only get called
 // when the last shared_ptr has been destroyed.
 //
 // Note also that _handle will keep the singleton
 // alive when there are no handles elsewhere
 // during execution.
 }
};
int main(int argc, char **argv)
{
 std::shared_ptr<my_singleton> hsingleton;
 try{
 hsingleton = singleton<my_singleton>::get();
 }catch(bad_singleton &){
 // blah initialization error
 }catch(...){
 // blah all other errors
 }
 return 0;
}

Using a template wrapper avoids the complexities associated with inheriting from "singletons" that implement their own initialization logic.

To me, it seems this can't break if used within one process.

• Loki Astari: Unfortunately it is broken in a couple of ways. 1) Double checked locking does not work correctly. 2) Order of initialization problems. Full review to come. But it is much easier to write a singleton than you have made it. Take a look here: stackoverflow.com/questions/1008019/c-singleton-design-pattern/… All you need is some locks and the link will work.
• InsertMemeNameHere: Thank you for taking the time. However, I am curious as to how it could be broken. "_handle" is presumably atomic (represented here as an std::shared_ptr), such that even if threads spuriously find it null, they'd still have to wait in the spinlock (not only that, it could never be a partial write). Unless something odd happens when reading from an address that has a pending write...
• InsertMemeNameHere: I'd also like to add that, unless the compiler was written by moldy radishes, it should allocate 4 (or 8) bytes in the executable's image for _spinlock and _ptr, and 1 byte for _bad_singleton, and fill them with 0s. That is, when the image is read into memory, the values for those variables were already 0: No initialization should be needed. If only atomic functions are used on an integer and a pointer (_InterlockedXXX intrinsics with MSC), there is no chance of an initialization interfering with the lock (unless the compiler generates initialization code for individual integers).
• InsertMemeNameHere: About edit: If forgot to release the spinlock in if(_handle)... and if(_bad_singleton)....
• InsertMemeNameHere: Also, I don't allow singletons to request the initialization of another singleton internally, unless the relationship is one-directional. Singletons effectively represent subsystems whose dependencies should form a DAG. If there are cycles in their dependency graph, it is always possible to break them by either coalescing two or more subsystems or introducing a "mediator" that does nothing but initialize the two at some point (handled through a specialization of the singleton template).

Answer 1

Your use of underscore is correct for variables. BUT the actual rules are complex enough, that using them at the start of an identifier is not a good idea. Do you know what all the rules for underscore as the first character of an identifier are? See this SO thread for an in-depth discussion.

You use _WriteBarrier(). This identifier is reserved by the implementation. If you have written this you need to change its name. If this is provided by your implementation its non portable and you are probably using it incorrectly. Any function begin with an underscore provided by the implementation is usually private and there is a function without the underscore that provides public access to this function. Use that.

Note: The compiler is no help and will not warn you when you do it incorrectly.

Order of construction problem:

// Once again, shared_ptr might require initialization logic
// that could run after the allocation in get() [assuming we have things
// running before main]. To make this robust, use a pointer here
// and OS-provided atomic functions.
static std::shared_ptr<singleton_t> _handle;

Don't use a pointer to solve it. It just makes the problem worse.

The best way to solve it is using a static member of a static method:

static std::shared_ptr<singleton_t>& getHandle()
{
 static std::shared_ptr<singleton_t> theHandle;
 return theHandle;
}

If you are worried about construction in a multi-threaded environment. Then add a lock. Note: In gcc it is already guaranteed thread safe but you will need code for other compilers.

Your write barriers:

 _frob = new singleton_t();
 _WriteBarrier();
 std::shared_ptr<singleton_t> _derp(_frob);
 _WriteBarrier();
 _handle = move(_derp);

Lets assume your _WriteBarrier() actually works. But there is no way to determine this as there is no code for it.

Your first write barrier is a waste of time (and just superfluous code) as derp is not accessed by any other code and can only be accessed by one thread (you have caught other threads in a trap).

The second _WriteBarrier() is where you need a barrier. But it still does not do anything useful as a shared pointer is not atomic or thread safe. Thus half way through the update of this variable (_handle) one of the other threads could escape your trap above result in code being executed using a variable in a non consistent state.

Template's instantiation of members is definitely a bad idea (when done in a header file).

template<typename singleton_t>
bool singleton<singleton_t>::_bad_singleton;
template<typename singleton_t>
std::atomic<uintptr_t> singleton<singleton_t>::_spinlock;
template<typename singleton_t>
std::shared_ptr<singleton_t> singleton<singleton_t>::_handle;

This basically creates an instance of each variable of each type that is used in every compilation unit. Though the linker will then consolidate multiple instances across compilation units for you when building a application (from simple object files) into a single variables. It will not do so correctly when building shared libraries. Because the standard has nothing to say about shared libraries. Shared libraries and the runtime linker do not have enough information to consolidate the variables across multiple shared libraries.

Thus each shared library that uses your code will have its own copy of these variables for each type that is used by the library. Thus which variable is used will depend on the library in which the code is called. Thus you can not build shared libraries using your code which makes it practically worthless.

Anyway your code is way over-complicated: The classic singleton design is much simpler (and safer): See this.

• Luc Danton: There is nothing wrong with the definitions (which are not instantiations) of the static data members of singleton, by virtue of it being a template. In any case inline can only be used with functions or function templates, not objects.
• user13931: That _WriteBarrier is certainly not his own function nor is it meant to be an implementation detail.
• InsertMemeNameHere: _WriteBarrier is an MS-specific compiler intrinsic.
• InsertMemeNameHere: Your suggestion is basically what is known as the Meyer's Singleton (not thread-safe). Furthermore, the C++ compilers I have access to DO NOT generate initialization guards. I have both MSC v10 and Intel C++ v12: Neither of which generate thread-safe static variable initialization guards (just a test and a jump). The code you suggest I use is completely unsafe for concurrent initializations.
• Loki Astari: I am sorry if you think this review is unhelpful. If you don't like me review feel free to ignore it. But it is broken: as I have pointed out in several places (Initialization Order/Shared_ptr is not atomic/Invalid for use in shared libraries). This is not a site for answers if you want a better solution ask on SO. But given the choice between a normal singelton (about 10 lines) verses 200 lines of code that is very hard to read (and thus maintain) I would use the normal version (as it is thread safe in C++11 and can easily be made thread safe in C++03 with a single lock) and is portable.