Bug in Singleton class

This is a discussion on Bug in Singleton class within the C++ Programming forums, part of the General Programming Boards category; Hi! I'm working with Visual Studio 2005 and have the following problem in my code: I have several Structs with ...

  1. #1
    Registered User
    Join Date
    Dec 2007
    Posts
    7

    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

  2. #2
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,893
    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.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  3. #3
    Registered User
    Join Date
    Dec 2007
    Posts
    7
    Quote Originally Posted by CornedBee View Post
    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?

  4. #4
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,893
    Yes. Reduce the problem to a minimal test case that exhibits it (tricky business - you'd have to remove code until the problem no longer occurs, then re-add the last thing you removed), then post that here.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  5. #5
    Registered User Codeplug's Avatar
    Join Date
    Mar 2003
    Posts
    4,702
    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

  6. #6
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,893
    It would be wrong. The compiler is supposed to merge all template implementations into a single one.

    On the other hand, the mechanism doesn't work across dynamic library boundaries. Do you have both a main program and a DLL, both using this mechanism?
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  7. #7
    Registered User
    Join Date
    Dec 2007
    Posts
    7
    Quote Originally Posted by Codeplug View Post
    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!

  8. #8
    Registered User
    Join Date
    Dec 2007
    Posts
    7
    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?

  9. #9
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    23,020
    Inline tells the compiler that you want to make the code inline (eg copy the code to wherever your function call is). The outcome of the executable doesn't change, but speed may become faster and size increase.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  10. #10
    Cat without Hat CornedBee's Avatar
    Join Date
    Apr 2003
    Posts
    8,893
    Only if the compiler cares.

    You're absolutely right that the inline keyword must never affect semantics. This still reeks of a compiler bug.
    All the buzzt!
    CornedBee

    "There is not now, nor has there ever been, nor will there ever be, any programming language in which it is the least bit difficult to write bad code."
    - Flon's Law

  11. #11
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    23,020
    Quote Originally Posted by CornedBee View Post
    Only if the compiler cares.
    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.
    Last edited by Elysia; 12-14-2007 at 10:44 AM.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  12. #12
    and the hat of sweating
    Join Date
    Aug 2007
    Location
    Toronto, ON
    Posts
    3,545
    Quote Originally Posted by Elysia View Post
    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.

  13. #13
    Registered User
    Join Date
    Dec 2007
    Posts
    7
    Quote Originally Posted by Elysia View Post
    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.

  14. #14
    C++まいる!Cをこわせ!
    Join Date
    Oct 2007
    Posts
    23,020
    Yes, in that case I suggest you try removing optimization and see if it helps. If it does, you can remove optimizations of single functions with pragmas (also disable specific optimizations for functions). See if you can narrow it down.
    Quote Originally Posted by Adak View Post
    io.h certainly IS included in some modern compilers. It is no longer part of the standard for C, but it is nevertheless, included in the very latest Pelles C versions.
    Quote Originally Posted by Salem View Post
    You mean it's included as a crutch to help ancient programmers limp along without them having to relearn too much.

    Outside of your DOS world, your header file is meaningless.

  15. #15
    The larch
    Join Date
    May 2006
    Posts
    3,573
    It is possible to attach code files?

    Are you sure you don't have undefined behaviour elsewhere in the code?
    I might be wrong.

    Thank you, anon. You sure know how to recognize different types of trees from quite a long way away.
    Quoted more than 1000 times (I hope).

Page 1 of 2 12 LastLast
Popular pages Recent additions subscribe to a feed

Similar Threads

  1. Compiler complaining about my singleton class
    By DavidP in forum C++ Programming
    Replies: 12
    Last Post: 06-09-2009, 03:30 AM
  2. Getting an error with OpenGL: collect2: ld returned 1 exit status
    By Lorgon Jortle in forum C++ Programming
    Replies: 6
    Last Post: 05-08-2009, 09:18 PM
  3. Creating a database
    By Shamino in forum Game Programming
    Replies: 19
    Last Post: 06-10-2007, 02:09 PM
  4. deriving classes
    By l2u in forum C++ Programming
    Replies: 12
    Last Post: 01-15-2007, 05:01 PM
  5. Abstract class problem
    By VanJay011379 in forum C++ Programming
    Replies: 9
    Last Post: 07-31-2002, 02:30 PM

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21