Joaquin M Lopez Munoz wrote:
Singletons (which is what Boost.Flyweight holder finally boil down to, see http://tinyurl.com/y996eqr ) are notoriously hard to get right, as explained for instance in Alexandrescu's "Modern C++ Design", and in the end one has to settle for some compromise. Boost.Flyweight uses holders in such a way that it guarantees initialization before main() begins, which for the most part avoids problems with lazy initialization in multithreaded environments.
Yes, I'm familiar with problems regarding singleton initialization and destruction. In fact, I'm quite satisfied with the current holder initialization (although, it could have used call_once to eliminate thread safety issues, but that's another topic).
Would you mind sketching what replacement for static_holder_class you have in mind?
I can imagine at least two ways of achieving the requested fix. The first one is to let the held factory to outlive the holder. Something along these lines: template< typename C > struct static_holder_class { static C& get() { static shared_ptr< C > instance; if (!instance) instance = make_shared< C >(); return *instance; } }; Or better yet: template< typename C > struct static_holder_class { static shared_ptr< C > get() { static once_flag flag = BOOST_ONCE_INIT; call_once(flag, &static_holder_class::get_impl); return get_impl(); } private: static shared_ptr< C > get_impl() { static shared_ptr< C > instance; if (!instance) instance = make_shared< C >(); return instance; } }; Now, the values created by factory should also hold a reference to the factory. In the first code snippet this would require C to derive from enable_shared_from_this. This will increase the value footprint a little, I know (one pointer, if intrusive_ptr is used). The second solution is to let the factory die, but to leave the values live as long as there are flyweights referencing them. This does not require changing the holder as its implementation is orthogonal to the factory nature. I can't say for sure what exact part of the library should be modified to achieve this, but at least the value factory and refcounted tracking policy would have to be changed. Boost.Intrusive would help to craft the container to store values in. Here is a snippet to get the idea: typedef intrusive::set_base_hook< intrusive::link_mode< intrusive::auto_unlink >, intrusive::optimize_size< true > > hook_t; template< typename ValueT > struct value_holder : public hook_t { atomic_count m_RefCount; ValueT m_Value; friend void intrusive_ptr_add_ref(value_holder*); friend void intrusive_ptr_release(value_holder*); }; template< typename ValueT > struct factory { typedef intrusive::set< value_holder, intrusive::base_hook< hook_t > > container_t; mutex m_Mutex; container_t m_Container; ~factory(); intrusive_ptr< value_holder< ValueT > > insert(ValueT const&); void erase(intrusive_ptr< value_holder< ValueT > > const&); }; The intrusive_ptr would have to be stored in the flyweights. The key points here would be release and ~factory. The destructor should call m_Container.clear(), which unlinks all values but leaves them valid. The release function, aside from decrementing the reference counter and destroying the value, has to check if the value is linked or not. If it is, the object is still in the factory container, and it should call factory::erase. If not, the value is not bound with the container and can be simply deleted.
PS: There's an additional approach that can solve the issue at hand, namely implementing a holder that never destroys the held object, but this is something I'd like to discuss with you after your original refcount proposal is dealt with.
Yes, that's a possible solution, but it would introduce a memory leak, which I'd like to avoid.