Re: [Boost-users] (A dynamic) Singleton using weak_ptr and shared_ptr

From: Martin T. (0xCDCDCDCD_at_[hidden])
Date: 2008-07-26 06:35:42

David Rodríguez Ibeas wrote:
> Hi Martin,
> 
>    After a first glance it seems strange that you are only requesting 
> the lock (sync_init) in get_instance(), which seems to be ok anyway 
> (after quite a lot of shared_ptr/weak_ptr internals). 

The weak_ptr::lock function will only be useful when the weak_ptr 
already references something. For the first initialization I'm facing a 
race condition btw. different threads in that concurrent execution of 
weak_ptr::lock would yield multiple _empty_ shared_ptr references.

> Now I wonder 
> whether the busy wait is needed or the combination of a mutex and a 
> condition variable could be better, waking the waiting thread just when 
> destruction of the object completes.
> 
Yes, I have been wondering if another synchronization method could be 
better, but (to my mind at least) I find the busy wait quite clear and I 
couldn't come up with anything more efficient/beautiful :)

>    Also I am not sure of the necessity of having both a 
> start_construction() and finish_construction() that require two mutex 
> acquisitions:

Hmmm ... yes, it seems it would not make any difference to have 
[the_object_exists = true;] at the end of start_construction. -- on the 
other hand, it's not constructed yet, so I would have to think of 
another name for the flag :)

> // Using boost::condition:
> struct dynamic_singleton::impl : private boost::noncopyable
> {  
>     impl() {}
>     ~impl() {}
>     static void start_construction()
>     {  
>         boost::recursive_mutex::scoped_lock lock( sync_ );
>         while ( the_object_exists ) {
>             cond_.wait( lock );
>         }
>         the_object_exists = true;
>     }
>     static void finish_destruction()
>     {  
>         boost::recursive_mutex::scoped_lock lock( sync_ );
>         the_object_exists = false;
>         cond_.notify_one();
>     }
>     typedef boost::weak_ptr<dynamic_singleton> internal_shared_t;
>     static internal_shared_t the_object;
> 
>     static boost::recursive_mutex sync_init;
>     static boost::recursive_mutex sync_;    // moved from atomic_bool
>     static bool the_object_exists;  // plain bool, synch'ed with sync_
>     static boost::condition cond_;
> };
> dynamic_singleton::impl::internal_shared_t 
> dynamic_singleton::impl::the_object;
> boost::recursive_mutex dynamic_singleton::impl::sync_init;
> boost::recursive_mutex dynamic_singleton::impl::sync_;
> bool dynamic_singleton::impl::the_object_exists = false;
> boost::condition dynamic_singleton::impl::cond_;
> 
>    Together with the rest of the code. As get_instance() warrants that 
> there is at most one process trying to create the object, and all 
> possibly deleting threads have already gone by (after the while loop in 
> start_construction() ) changing the bool here or later is just the same.
> 

Oh my. Yes this seems to work. After I have wrapped my head around the 
condition_variable usage pattern. :) (This solution seems to need more 
in-code comments though, or else I wont understand it two weeks from now :)

>    One other comment, I would mark both constructor and destructors for 
> impl as private, since all methods and data are static there is no point 
> in constructing objects of this class. This requires modifying 
> dynamic_singleton class to remove the pimpl attribute.
> 

Yes, you are right. In this example there's no point to constructing 
this class.

cheers,
Martin