Re: question re. usage of "static" within static member functions of
a class
On Sep 9, 5:15 pm, "Chris M. Thomasson" <n...@spam.invalid> wrote:
"James Kanze" <james.ka...@gmail.com> wrote in message
news:f0ed97f9-ea04-4952-88f2-a1982725a0b4@38g2000yqr.googlegroups.com...
On Sep 7, 7:24 am, "Chris M. Thomasson" <n...@spam.invalid> wrote:
"ssb" <s.sharm...@gmail.com> wrote in message
news:97dc452a-f6a5-4a77-9a9c-ea8491d37e40@e4g2000prn.googlegroups.com..=
..
During a code review, I found the following lines of code:
[...]
The "instance" method was implemented as follows:
Data* Data::instance()
{
static Data* model = new Data();
return model;
}
I have never come across a situation where a pointer was set
to static in such a case. Is this valid?
It's a singleton.
And to answer the question, it's perfectly valid. A pointer is
an object, just like any other variable, and obeys the same
rules as other variables.
What are the potential pitfalls in such programming practices?
The storage that `model' points to will never be destroyed,
also it's not thread-safe.
Not being destroyed is presumably the reason the code is written
this way. Most of the time, you don't want a singleton to be
destructed. In other word, it's a feature designed to avoid
pitfalls. As for thread-safety, it depends on the
implementation, it is thread safe---modulo bugs---in g++. (But
you're probably right for most implementations.)
You can get around static initialization and destruction ordering issues =
by
using a strongly thread-safe smart pointer to manage the singleton. The
pseudo-code would be something like the following pseudo-code:
_____________________________________________________________________
template<typename T>
static local_ptr<T>
once()
{
static global_ptr<T> g_instance;
local_ptr<T> lptr(g_instance);
if (! lptr)
{
static pthread_mutex_t g_mutex = PTHREAD_MUTEX_INITIALI=
ZER;
pthread_mutex_lock(&g_mutex);
lptr = g_instance;
if (! lptr)
{
try
{
lptr.reset(new T());
}
catch (...)
{
pthread_mutex_unlock(&g_mutex);
throw;
}
g_instance = lptr;
}
pthread_mutex_unlock(&g_mutex);
}
return lptr;}
_____________________________________________________________________
This is strongly thread-safe and will always work no matter how the stati=
c
ctor/dtor ordering comes out. The caveat, well, it's definitely not as
efficient as using a raw pointer and explicitly leaking the singleton.
There are so many things wrong with that code sample, I don't even
know where to start. (Exaggeration. I do know where to start.)
Firstly and most importantly, you're using double checked locking,
which is broken in effectively all C++ implementations. Don't do that.
Please read, continue to re-read if you don't get it, this excellent
paper:
http://www.aristeia.com/Papers/DDJ_Jul_Aug_2004_revised.pdf
Next, you also have a race condition on the construction of the mutex
itself, in addition to the double checked locking of the singleton
instance construction. PTHREAD_MUTEX_INITIALIZER is entirely
equivalent to calling pthread_mutex_init, and thus is not thread-safe.
Repeat:
lptr.reset(new T());
is not properly guarded (double checked locking) and
static pthread_mutex_t g_mutex = PTHREAD_MUTEX_INITIALIZER;
is not properly guarded (simple race condition). Both are race
conditions when the first call of this function may be concurrent.
Also, users may want multiple singletons of the same type. One unit of
code makes a singleton of type T, and another piece of code entirely
separate will make another singleton of type T. Your code does not
support that. Instead, you will get exactly 1 instance of T across the
entire program. This is a significant limitation. You could
potentially get it around it with something like:
//header code
template <typename singleton_t, typename once_type>
singleton_t & getSingleton();
//code using the header to make a singeton in a hpp
class singleton_x_type { /* ... */ };
class once_type_for_singleton_X {};
singleton_x_type & getSingletonX()
{
return getSingleton<singleton_x_type, once_type_for_singleton_X>
();
}
If another piece of code wanted their own singleton of type X, then
they could define their own once_type and instantiate getSingleton on
that new once_type.
Lastly, RAII is your friend. Don't ever explicitly call
pthread_mutex_lock or pthread_mutex_unlock except in your portability
layer mutex wrapper class. This is not a correctness issue per se, but
vastly improves the chances that the code will be correct.
Your solution could be made correct by eager initializing it by adding
namespace { bool force_init = (::once<your_type>(), true); }
However, at that point, your fixed code is pretty much equivalent to
several examples already posted, aka:
T& getSingleton()
{ static T* x = new T;
return *x;
}
namespace { bool force_init = (getSingleton(), true); }