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
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