SingletonPtr - name not accurate, thread-safety and initialization/destruction

[0xc0170]

1. The name is not accurate
   The classes name SingletonPtr is confusing. This is not a singleton. We can create multiple instances of the same class. Therefore similar pattern in another code base is called LazyInstance for example.

2. Thread safety
   This is not thread-safe, is it?:

           if (NULL == _ptr) {
           singleton_lock();
           _ptr = new (_data) T;
           singleton_unlock();
   }
   

3. Initialization of T type
   It's not clear what SingletonPtr guarantees regarding initialization of a T object.

           if (NULL == _ptr) {
               singleton_lock();
               _ptr = new (_data) T;
               singleton_unlock();
   }
   

   If T is POD, its data are not initialized (at least C++03,C++98). should it be _ptr = new (_data) T(); ?

4. The destructor for the T object
   There could be a method to destroy T object (call explicitly dtor).

Code reference: https://github.com/ARMmbed/mbed-os/blob/d89c3c18f9be97880f9173e87772ea2808fa8df1/hal/api/SingletonPtr.h

@pan- @c1728p9 @geky @sg-

[geky]

+1 for LazyInstance

The initializer should probably be updated to:

if (NULL == _ptr) {
    singleton_lock();
    if (NULL == _ptr) {
        _ptr = new (_data) T();
    }
    singleton_unlock();
}

As for destructors, can they not already call the destructor manually?

LazyInstance<Thingy> lazy_thingy;
lazy_thingy->do_thingy();
lazy_thingy->~Thingy();

I vote for not allowing reconstructing after destructing, since I believe that requires locking over all class member functions for thread safety.

[c1728p9]

What @geky says sounds good. Renaming sounds good to me, and both _ptr = new (_data) T(); and the extra if (NULL == _ptr) { inside the lock are needed.

[pan-]

@geky For the sake of symmetric API it would be better to have a function which call the destructor and reset the pointer. Otherwise, what happen if the get is called after the object has been destructed ? Undefined behavior is an option but we can do better.

Other points that worry me with the current implementation:

• How to construct objects with no default constructor ? There should be a way to initialize objects without default constructors.
• It is not possible to add a private copy constructor or assignment operator otherwise LazyInstance will not be a POD but it should be documented and users warned about it.
• Const correctness can be improved and const overloads of get and operator-> can be provided if the data members are marked as mutable.
• From a semantic point of view, why get return a pointer, it makes a lot of sense if the class is named SingletonPtr but much less if the class is named LazyInstance ? Pointers can be NULL references can't and are guarantee to work. It would be interesting to have a clear guideline for pointers and references usage, there is a lot of valid reasons to use one or the other.
• It would be interesting to customize the locking part with a trait. It would allow users to use their own mutex or no mutex at all.

About the mutex part, I have a question, it seems that we embed more and more mutexes in classes rather than outside class is it a design pattern we want to follow ? I deeply fear that it strike back, mutexes does not compose well by nature, by pilling up hidden mutexes, we just increase the risk of deadlock, lock contention and priority inversion. All that totally hidden to the user and not easily fixable since it's part of the classes.

Edit: apparently RTX employs priority inheritance for mutexes so priority inversion is out of the scope of inner mutexes.

[geky]

Would it be ok to reset the pointer without synchronizing against calls to the class? Otherwise the only options for synchronizing I can think of are locking over all calls to the class (very deadlock prone) or some sort of buffered RCU lock (at minimum 2x memory consumption and complex).

+1 for const correctness. Unfortunately class ordering rules will prevent placement in rom while the initialized flag is there, but we can dream.

[c1728p9]

I created #2503 which addresses issues 2 and 3 that @0xc0170 mentioned.

[ciarmcom]

ARM Internal Ref: IOTMORF-419

[c1728p9]

Thanks for the ideas @pan-. My response to your questions is below. I also created #2546 to implement these.

Construction / Destruction - Ideally I would like SingletonPtr to behave the same as a global class, just one which is used lazily. This means initialized on first use and destroyed program exit but only if initialized. Since calling destructors on program exit is undefined behavior and mbed-os code never exits under normal circumstances (when main returns), the destruction probably does not matter too much. Maybe for completeness when/if initialization occurs the object is added to the list of destructors that gets called on program exit?

-Because of the way the lazy initialization is done it makes it hard to pass in arguments on construction. Any ideas on how to make this easier would be appreciated. Right now this has to be done by extending the class and making a default constructor pass in the desired args. This could also be done with template parameters, but I figured adding that might lead to error prone code if the template parameters are non constant values. Depending on when the singleton is first accessed the args used in construction could be different.
-There is a note in the code that this class should only be declared in static context (do not create on the stack or with new). In addition, if the copy construct or assignment operator is used an assert is be triggered on first use, as _ptr would be non-null and pointing to the wrong data. Is this sufficient or do you want me to add more documentation on this?
-I didn't know C++ had a mutable keyword. Thanks for pointing this out. I created PR #2546 for this.
-I had this return a pointer so it matched "operator->" and the class name. Aside from that I don't really have preference whether this is a pointer or reference.
-How would this look? I'm not really sure what you want here. If you could add a comment to #2546 or make a simple PR for this I could provide feedback on it.

As for the mutex, the initial draft I had used dynamic memory and did not require a mutex as the pointer was set atomically. The problem with that is classes could get created multiple times on on first use and all but one would be destroyed. After some discussion with Chris we decided that using placement new and protecting initialization by a mutex would be less intrusive. As long as the constructors are kept simple deadlocks shouldn't be a problem. I'm open to ideas on this issue though, as both implementations that were discussed have drawbacks.

[bridadan]

-I had this return a pointer so it matched "operator->" and the class name. Aside from that I don't really have preference whether this is a pointer or reference.

@c1728p9 I will say if I had the option to return a reference, it would definitely help in some cases in refactoring utest to increase linker-code-removal-friendliness. I found this page describing lazily initialized singletons that can return a reference, not sure if it helps at all: http://www.devarticles.com/c/a/Cplusplus/C-plus-plus-In-Theory-The-Singleton-Pattern-Part-I/4/

[c1728p9]

Is there any consensus on name? Should this be renamed to LazyInstance? Sounds like @0xc0170 and @geky are for this. @sg-, @pan- what are your thoughts. Let me know and I'll update it.

[sg-]

I didn't think SingletonPtr was that bad. Its returns a pointer to a singleton.

[geky]

Though not a pointer to a singleton of a class, and the class itself isn't really a pointer since it contains the object and should never be copied.

I like LazyInstance, it describes what the class does, which is to instantiate an object lazily.

[sg-]

Should a singleton ever be copied?

The class overrides the dereference operator so semantically acts like pointer to an object?

I dont care that much, just pick something, document it and go with it. If there is precedent in other frameworks of the same language, follow suit.

[sg-]

Ok - decision made. We have better things to do than rename things that arent that bad. I'll call bikeshed and close this out.