Thread: Bug in Singleton class

Hi!

I'm working with Visual Studio 2005 and have the following problem in my code:

I have several Structs with member variables. Those Structs are used by several classes as "Parameter classes", so if a member variable is changed, all classes should get the changed variable the next time they call it.
To implement this behaviour I used a Singleton Holder class. Here is the layout of the classes:

In SingletonHolder.h

Code:

template<class T>
class SingletonHolder
{
public:
  static T& Instance();

private:
  SingletonHolder();
  SingletonHolder(const T&);
  SingletonHolder& operator=(const T&);
  ~SingletonHolder();
};

template<class T>
T& SingletonHolder<T>::Instance()
{
  static T obj;
  return obj;
}

The Parameter classes are defined in parameter.h

Code:

struct Params
{
private:
  double m_val1;
  double m_val2;

public:
  double getVal1() const { return m_val1; }
  double getVal2() const { return m_val2; }
  void setVal1(double val) { m_val1 = val; }
  void setVal2(double val) { m_val2 = val; }

  Params();
};

struct OtherParams
{
private:
  double m_otherVal1;
  double m_otherVal2;

public:
  double getOtherVal1() const { return m_otherVal1; }
  double getOtherVal2() const { return m_otherVal2; }
  void setOtherVal1(double val) { m_val1 = val; }
  void setOtherVal2(double val) { m_val2 = val; }

  OtherParams();
};

// only those Types are used by Client classes:
typedef SingletonHolder<Params> SingletonParams;
typedef SingletonHolder<OtherParams> SingletonOtherParams;

The implementation of Params and OtherParams in parameter.cpp:

Code:

Params::Params()
: m_val1(1)
, m_val1(2)
{
  // should be called only once (when static object is created)!
  cout << "Calling Params()" << endl;
}

OtherParams::OtherParams()
: m_val1(10)
, m_val1(20)
{
  // should be called only once (when static object is created)!
  cout << "Calling OtherParams()" << endl;
}

Classes can now access those parameters through:

Code:

double val1 = SingletonParams::Instance().getVal1();
double val2 = SingletonOtherParams::Instance().getOtherVal2();
...

Please note, that there should be during program execution only exactly one instance of Params and OtherParams!

My Problem is, that in Debug Mode (in VS2005) everything works as expected but in Release Mode there is one function where suddenly a new Object is created (constructor was called and a second cout notification happens). The code looks like this:

Code:

SingletonParams::Instance().setVal1(5);
...
cout << SingletonParams::Instance().getVal1(); // OK! is 5

updateSomething(); // Still OK in this function

// first for loop
for (...)
{
    cout << SingletonParams::Instance().getVal1(); // Still OK.
}

// some other loop
for (...)
{
    cout << SingletonParams::Instance().getVal1(); // Ups, new Object and c'tor is called!
}

It doesn't matter how the loops look like the thing is the c'tors of Params and OtherParams should never be called twice. As I said problems start in Release Mode and not in Debug mode.

Is there something fundamentally wrong with my code? Are the typedefs in the right place? I now that there are other possibilities to implement the wanted behaviour but I'd like to now what my mistake was.

Maybe somebody has an idea?

Thanks!

Marquito

Originally Posted by CornedBee

Make the Params constructor private and friend SingletonHolder<Params>. Then the compiler should tell you if any other place in your code wants to default-construct a Params.

Nice trick. I did that but compilation was fine.

Any other thoughts?

Code:

template<class T>
class SingletonHolder
{
public:
  static T& Instance();

private:
  SingletonHolder();
  SingletonHolder(const T&);
  SingletonHolder& operator=(const T&);
  ~SingletonHolder();
};

template<class T>
inline T& SingletonHolder<T>::Instance()
{
  static T obj;
  return obj;
}

I would try adding the "inline" there. Multiple translation units that include that header could be generating more than one instance of that function, thus more than one instance of "static T obj". I don't know if the compiler would be right or wrong in doing so however.

gg

Originally Posted by Codeplug

Code:

template<class T>
class SingletonHolder
{
public:
  static T& Instance();

private:
  SingletonHolder();
  SingletonHolder(const T&);
  SingletonHolder& operator=(const T&);
  ~SingletonHolder();
};

template<class T>
inline T& SingletonHolder<T>::Instance()
{
  static T obj;
  return obj;
}

I would try adding the "inline" there. Multiple translation units that include that header could be generating more than one instance of that function, thus more than one instance of "static T obj". I don't know if the compiler would be right or wrong in doing so however.

gg

Thanks, that did the trick. Could somebody explain to me why? Is it a bug in the compiler?

There was no other dll using this mechanism. There were three classes using this mechanism. In 2 it fully worked but in the third one only until it reached the middle of a function. There the compiler must have created a new instance (because of optimization?).

Thanks for helping!

One last question:

Am I right when i say that a program shouldn't change its behaviour when the keyword inline is added or removed?

Originally Posted by Elysia

Yes, hence "may" become bigger and faster.

marquito:
This is obviously not your main code - why don't you provide your code so we can test?
The stub you provided does not exhibit the problem you are describing.
Also try a rebuild to see if that works.

or bigger & slower (because of page faults) if the optimizer is having a bad day.

Originally Posted by Elysia

marquito:
This is obviously not your main code - why don't you provide your code so we can test?
The stub you provided does not exhibit the problem you are describing.
Also try a rebuild to see if that works.

Sorry but its impossible. The main code is a few 1000 lines long... and the slightest change removes the bug so i can't give you a minimal example.
Without the inline fix a simple loop like

Code:

for(int i=0; i<10; ++i)
{
}

"triggers" the new instantiation - if commented out the behaviour is fine. I tried a rebuild but nothing changed. I can try to turn off optimization and see if anything changes.