Any memory leak for this Singleton Implementation

Discussion in 'C++' started by tomgee, Jan 13, 2006.

  1. tomgee

    tomgee Guest

    I saw it many times,

    // T.h:

    class T
    {
    public:
    static T* instance();
    private:
    T() {}
    ~T() {}
    static T* smInstance;
    };

    // T.cpp:

    T* T::smInstance = NULL;

    T* T::instance()
    {
    if (smInstance == NULL)
    smInstance = new T();

    return smInstance;
    }

    But I suspected there is a memory leak, because there is a free() call
    for the memory allocated by new().

    However, I saw it many times that I tend to doubt myself - a similiar
    implementation even appears on the "c++ cook book", a very new book by
    O'really.

    Anybody can help to clarify? Thanks.
     
    tomgee, Jan 13, 2006
    #1
    1. Advertising

  2. tomgee

    mlimber Guest

    tomgee wrote:
    > I saw it many times,
    >
    > // T.h:
    >
    > class T
    > {
    > public:
    > static T* instance();
    > private:
    > T() {}
    > ~T() {}
    > static T* smInstance;
    > };
    >
    > // T.cpp:
    >
    > T* T::smInstance = NULL;
    >
    > T* T::instance()
    > {
    > if (smInstance == NULL)
    > smInstance = new T();
    >
    > return smInstance;
    > }
    >
    > But I suspected there is a memory leak, because there is a free() call
    > for the memory allocated by new().
    >
    > However, I saw it many times that I tend to doubt myself - a similiar
    > implementation even appears on the "c++ cook book", a very new book by
    > O'really.
    >
    > Anybody can help to clarify? Thanks.


    I think you mean something more like:

    template<class T>
    class Singleton
    {
    // ...
    };

    It's not a memory leak if your program never stops running or if the OS
    cleans up after you, one of which is probably true on all modern
    systems. See chapter 6 of _Modern C++ Design_ for more than you ever
    wanted to know about implementing singletons in C++.

    Cheers! --M
     
    mlimber, Jan 13, 2006
    #2
    1. Advertising

  3. tomgee

    tomgee Guest

    No, it's not templated. I am sorry to have used this misleading class
    name.

    How can you make sure the OS will clean it up, without calling a
    free()? Can you please explain a little implementation here, I am only
    concerned about the dynamic memory allocation and claiming?

    Thanks.
     
    tomgee, Jan 13, 2006
    #3
  4. tomgee

    Ben Pope Guest

    tomgee wrote:
    > I saw it many times,
    >
    > // T.h:
    >
    > class T
    > {
    > public:
    > static T* instance();
    > private:
    > T() {}
    > ~T() {}
    > static T* smInstance;
    > };
    >
    > // T.cpp:
    >
    > T* T::smInstance = NULL;
    >
    > T* T::instance()
    > {
    > if (smInstance == NULL)
    > smInstance = new T();
    >
    > return smInstance;
    > }
    >
    > But I suspected there is a memory leak, because there is a free() call
    > for the memory allocated by new().


    I suspect you meant there is *NO* call to free.

    What you *should* have said is that there is no call to delete.

    > However, I saw it many times that I tend to doubt myself - a similiar
    > implementation even appears on the "c++ cook book", a very new book by
    > O'really.
    >
    > Anybody can help to clarify? Thanks.


    When is smInstance deconstructed?

    Ben Pope
    --
    I'm not just a number. To many, I'm known as a string...
     
    Ben Pope, Jan 13, 2006
    #4
  5. tomgee wrote:
    > I saw it many times,
    >
    > // T.h:
    >
    > class T
    > {
    > public:
    > static T* instance();
    > private:
    > T() {}
    > ~T() {}
    > static T* smInstance;
    > };
    >
    > // T.cpp:
    >
    > T* T::smInstance = NULL;
    >
    > T* T::instance()
    > {
    > if (smInstance == NULL)
    > smInstance = new T();
    >
    > return smInstance;
    > }


    Seems like this 'T' is created once and deleted never.

    > But I suspected there is a memory leak, because there is a free() call
    > for the memory allocated by new().


    Huh? Care to rephrase that?

    > However, I saw it many times that I tend to doubt myself - a similiar
    > implementation even appears on the "c++ cook book", a very new book by
    > O'really.


    O'Reilly. Unless you intended a pun (word play on "Oh, really?")

    On many modern platforms, when the program ends, all of the memory it
    occupied is released. If something is dynamically created and never
    destroyed explicitly, it is _quite_possible_ that the memory is still
    made available to the system by the system after the program ends. But
    you need to consider a couple of other points. First, what if memory
    is not released? What if somebody writes a memory management engine
    that keeps memory around forever until the user explicitly deletes it?
    In that case the rule "always 'delete' what you get from 'new'" has all
    the merit it can ever have. Second, what if somebody fixes this code
    in such a way that the destructor of the singleton now has side effects?
    If you don't explicitly 'delete' the pointer, the side effects will never
    take place. And so on...

    V
     
    Victor Bazarov, Jan 13, 2006
    #5
  6. tomgee

    TB Guest

    tomgee sade:
    > No, it's not templated. I am sorry to have used this misleading class
    > name.
    >
    > How can you make sure the OS will clean it up, without calling a
    > free()? Can you please explain a little implementation here, I am only
    > concerned about the dynamic memory allocation and claiming?
    >
    > Thanks.
    >


    The OS always reclaims the memory your program allocated upon exit,
    wheather you delete'd it or not.

    But if you want too, you could delete it yourself when your program
    exits:

    void foo() {
    delete T::instance();
    }

    #include <cstdlib>

    int main() {
    std::atexit(&foo);
    return 0;
    }

    TB
     
    TB, Jan 13, 2006
    #6
  7. tomgee

    tomgee Guest

    Thank you all for the above threads. I was in a hurry and made some
    typos and mistakes, but you are smart to understand my question through
    all that.

    Since "The OS always reclaims the memory your program allocated upon
    exit,
    wheather you delete'd it or not. ", can I say, generally, memory leak
    only happens when the application is running(it keeps asking more
    memory without returning), and ends all when it terminates? Well,
    Suppose the occasions Victor Bazarov mentioned won't come up.

    Thanks
     
    tomgee, Jan 13, 2006
    #7
  8. tomgee

    Markus Moll Guest

    Hi

    tomgee wrote:

    > But I suspected there is a memory leak, because there is a free() call
    > for the memory allocated by new().


    Well, it's at least bad practice, as I would expect the destructor of the
    singleton object to be called before the program exits.

    Anyway, I just wanted to add that a solution would be to simply use
    std::auto_ptr (making std::auto_ptr friend).

    Markus
     
    Markus Moll, Jan 13, 2006
    #8
  9. tomgee

    Jay Nabonne Guest

    On Fri, 13 Jan 2006 12:24:34 -0800, tomgee wrote:

    > I saw it many times,
    >
    > // T.h:
    >
    > class T
    > {
    > public:
    > static T* instance();
    > private:
    > T() {}
    > ~T() {}
    > static T* smInstance;
    > };
    >
    > // T.cpp:
    >
    > T* T::smInstance = NULL;
    >
    > T* T::instance()
    > {
    > if (smInstance == NULL)
    > smInstance = new T();
    >
    > return smInstance;
    > }
    >
    > But I suspected there is a memory leak, because there is a free() call
    > for the memory allocated by new().
    >


    Not only does it leak, but the destructor won't be called.

    If you don't use a pointer, you can make it auto-destructing:

    class T
    {
    public:
    static T& instance();
    private:
    T() {}
    ~T() {}
    };

    // T.cpp:

    T& T::instance()
    {
    // constructed when first executed. Destructed when program exits.
    static T t;

    return t;
    }

    - Jay
     
    Jay Nabonne, Jan 14, 2006
    #9
  10. tomgee

    Axter Guest

    Victor Bazarov wrote:
    > tomgee wrote:
    > > I saw it many times,
    > >
    > > // T.h:
    > >
    > > class T
    > > {
    > > public:
    > > static T* instance();
    > > private:
    > > T() {}
    > > ~T() {}
    > > static T* smInstance;
    > > };
    > >
    > > // T.cpp:
    > >
    > > T* T::smInstance = NULL;
    > >
    > > T* T::instance()
    > > {
    > > if (smInstance == NULL)
    > > smInstance = new T();
    > >
    > > return smInstance;
    > > }

    >
    > Seems like this 'T' is created once and deleted never.
    >
    > > But I suspected there is a memory leak, because there is a free() call
    > > for the memory allocated by new().

    >
    > Huh? Care to rephrase that?
    >
    > > However, I saw it many times that I tend to doubt myself - a similiar
    > > implementation even appears on the "c++ cook book", a very new book by
    > > O'really.

    >
    > O'Reilly. Unless you intended a pun (word play on "Oh, really?")
    >
    > On many modern platforms, when the program ends, all of the memory it
    > occupied is released. If something is dynamically created and never
    > destroyed explicitly, it is _quite_possible_ that the memory is still
    > made available to the system by the system after the program ends. But
    > you need to consider a couple of other points. First, what if memory
    > is not released? What if somebody writes a memory management engine
    > that keeps memory around forever until the user explicitly deletes it?
    > In that case the rule "always 'delete' what you get from 'new'" has all
    > the merit it can ever have. Second, what if somebody fixes this code
    > in such a way that the destructor of the singleton now has side effects?
    > If you don't explicitly 'delete' the pointer, the side effects will never
    > take place. And so on...
    >


    Having a singleton get explicitly deleted can cause more harm then
    good, if it's called from a destructor of multiple global objects.

    If you try to delete it in main, then your global objects will be
    calling a deleted object, and at best, it will result in undefined
    behavior, and at worse crash the program.

    So it's not ALWAYS preferable to have a singleton get explicitly
    deleted, and that's why you'll commonly find singleton implementation
    that never calls the destructor.
     
    Axter, Jan 14, 2006
    #10
  11. tomgee

    Axter Guest

    Jay Nabonne wrote:
    > On Fri, 13 Jan 2006 12:24:34 -0800, tomgee wrote:
    >
    > > I saw it many times,
    > >
    > > // T.h:
    > >
    > > class T
    > > {
    > > public:
    > > static T* instance();
    > > private:
    > > T() {}
    > > ~T() {}
    > > static T* smInstance;
    > > };
    > >
    > > // T.cpp:
    > >
    > > T* T::smInstance = NULL;
    > >
    > > T* T::instance()
    > > {
    > > if (smInstance == NULL)
    > > smInstance = new T();
    > >
    > > return smInstance;
    > > }
    > >
    > > But I suspected there is a memory leak, because there is a free() call
    > > for the memory allocated by new().
    > >

    >
    > Not only does it leak, but the destructor won't be called.
    >
    > If you don't use a pointer, you can make it auto-destructing:
    >
    > class T
    > {
    > public:
    > static T& instance();
    > private:
    > T() {}
    > ~T() {}
    > };
    >
    > // T.cpp:
    >
    > T& T::instance()
    > {
    > // constructed when first executed. Destructed when program exits.
    > static T t;
    >
    > return t;
    > }


    If you use the above method, and the singleton gets called by a global
    object's destructor that's in a different translation unit, it will
    result in undefined behavior, because there's no garantee of the order
    of destruction for global objects in multiple translation units, IAW
    C++ standards.

    If the singleton is not called from any global object's destructor,
    then the above implementation would be the safer and cleaner method.
     
    Axter, Jan 14, 2006
    #11
  12. tomgee

    JustBoo Guest

    On 13 Jan 2006 23:48:51 -0800, "Axter" <> wrote:
    >If you use the above method, and the singleton gets called by a global
    >object's destructor that's in a different translation unit, it will
    >result in undefined behavior, because there's no garantee of the order
    >of destruction for global objects in multiple translation units, IAW
    >C++ standards.
    >
    >If the singleton is not called from any global object's destructor,
    >then the above implementation would be the safer and cleaner method.


    Thank you. That is the first rational explanation that I can
    *remember* to counter all the hand-wringing that goes with
    Singletons. Mind you, I have felt queasy at times relying on an
    unreliable OS to clean up myself. "But we have deadlines to
    meet dammit." :) It's always a balance.

    I've used them for years with no trouble except the angst from other
    engineers. I've used them to help get younger engineers design's
    up and running quickly.

    I'll 'nick' the above if you don't mind. :)
     
    JustBoo, Jan 14, 2006
    #12
    1. Advertising

Want to reply to this thread or ask your own question?

It takes just 2 minutes to sign up (and it's free!). Just click the sign up button to choose a username and then you can ask your own questions on the forum.
Similar Threads
  1. Proton Projects - Moin

    Singleton - Whether Cloneable overrides Singleton

    Proton Projects - Moin, Mar 26, 2007, in forum: Java
    Replies:
    4
    Views:
    3,259
    Proton Projects - Moin
    Mar 27, 2007
  2. Wilhelm
    Replies:
    1
    Views:
    167
  3. Trans
    Replies:
    12
    Views:
    280
    Robert Klemme
    Sep 14, 2007
  4. Paul McMahon
    Replies:
    3
    Views:
    207
    David A. Black
    Jun 9, 2008
  5. Charles Oliver Nutter

    Singleton methods without the singleton class

    Charles Oliver Nutter, Mar 15, 2010, in forum: Ruby
    Replies:
    4
    Views:
    204
    Charles Oliver Nutter
    Mar 22, 2010
Loading...

Share This Page