[signals2][review] The review of the signals2 library (formerly thread_safe_signals) begins today, Nov 1st
Hello all, The review for the Signals2 library (formerly known as thread_safe_signals) submitted by Frank Mori Hess begins today (Nov 1st) and is scheduled to end on Nov 10th. I would like to thank Franz Alt, Terry Golubiewski, Doug Gregor, Ravikiran Rajagopal and Andrew Webber for making this review possible by committing to reviewing the library. How to submit a review: -------- As usual, EVERYONE is welcome to participate in the review discussions and to submit a review. I strongly encourage participation from reviewers that would examine the library from a purely user standpoint (commenting on the interface and / or the documentation), as well as reviewers that would be willing to look into the details of the implementation (i.e., you don't have to focus on both). Here are some questions you might want to answer in your review (feel free to skip those that don't apply to your analysis): * What is your evaluation of the design? * What is your evaluation of the implementation? * What is your evaluation of the documentation? * What is your evaluation of the potential usefulness of the library? * Did you try to use the library? With what compiler? Did you have any problems? * How much effort did you put into your evaluation? A glance? A quick reading? In-depth study? * Are you knowledgeable about the problem domain? And finally, every review should answer this question: * Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion. Please submit your review either to the list or privately to me by the end of the review period. If you send me a review privately I will forward it to the list so the review can be discussed. If you would prefer me to forward your review anonymously (with your name removed) please indicate that in your e-mail. If you are a first time reviewer, here is some more information to get you started: * http://www.boost.org/community/reviews.html has more information on the review process * the review officially takes place on both the boost dev and boost-user lists, but typically more of the discussion happens on the boost dev list * if you want to get a feel for past reviews, you can find them in the archives (see http://www.boost.org/community/groups.html) - the past review dates are on the review schedule (http://www.boost.org/community/review_schedule.html). Typically, there is a period of discussion between the reviewers and the author, with official reviews coming in towards the tail end of the review period. About the library: -------- * The library can be downloaded from: http://www.boostpro.com/vault/index.php?&directory=thread_safe_signals (latest version is signals2-2008-10-08.zip) * Documentation is also available online: http://www.comedi.org/projects/signals2/libs/signals2/doc/html/index.html * A synopsis: The Boost.Signals2 library (formerly known as thread_safe_signals) is an implementation of a managed signals and slots system. Signals represent callbacks with multiple targets, and are also called publishers or events in similar systems. Signals are connected to some set of slots, which are callback receivers (also called event targets or subscribers), which are called when the signal is "emitted." Signals and slots are managed, in that signals and slots (or, more properly, objects that occur as part of the slots) can track connections and are capable of automatically disconnecting signal/slot connections when either is destroyed. This enables the user to make signal/slot connections without expending a great effort to manage the lifetimes of those connections with regard to the lifetimes of all objects involved. When signals are connected to multiple slots, there is a question regarding the relationship between the return values of the slots and the return value of the signals. Boost.Signals2 allows the user to specify the manner in which multiple return values are combined. * Relationship to Boost.Signals: This is a thread-safe variant of the original Boost.Signals library. There have been some changes to the interface to support thread-safety, mostly with respect to automatic connection management. The following thread offers some more details on the differences between the two implementations, as well as a plan of a phased replacement of Boost.Signals should Signals2 be accepted: http://tinyurl.com/4sqau3 [nabble] The Signals2 FAQ also covers some differences: http://tinyurl.com/576gl6 Kind regards, Stjepan (review manager)
Hi, I consider this submission a evolutionary development of the signals library, so I'd like to focus the review questions on the parts that changed. Stjepan Rajko schrieb: [...]
Here are some questions you might want to answer in your review (feel free to skip those that don't apply to your analysis):
* What is your evaluation of the design?
* Automatic connection management: This is great, even if it wasn't required for thread safety reasons, I prefer using shared_ptr based connection tracking over boost::trackable, since it enables me to track (and thus safely use) 3d party classes. So on that note I would suggest to strike the "Unfortunately" from the Auto Connection Management Design Rationale page, and emphasize the benefit of the new design. * Namespace signals2: Under normal circumstances I would argue for keeping the name "signals" for the namespace, because frankly its the best name for the concept. But considering the real-world constraint of staying compatible with code using Qt, any other name is much preferable. (I consider this a deal-breaker, since using "signals" forces you to either patch boost, or to educate your users of the Issue and have them deal with the problem) * I haven't realy understood the connect_extended / extended_slot_type from the documentation.
* What is your evaluation of the implementation?
Just skimmed through it,
* What is your evaluation of the documentation?
I haven't done a comparison, but the Docu seems to be mostly the same as the original signals. Thus for the usecases that are documentated there its high quality, but I miss the same for the new features: * I miss tutorial quality examples for some of the new features: ** Howto use boost::threads with signals2 ** Howto use postconstructors/predestructors (and what for?) ** An example using signal::extended_slot_type * The class reference pages (e.g. doc/html/boost/signals2/connection.html) are missing from the TOC * Do the portable syntax examples need to be supported (... so prominently)? Maybe they could be moved to an appendix? * The class/function reference pages format makes it hard to read. It would be nice if they used a stylesheet like doxygen does (see for example this page: http://xerces.apache.org/xerces-c/apiDocs-3/classDOMDocument.html , it makes it much easier to see which function is being documented by any given text.) (This nit is not specific to signals2, but while I'm at it ...)
* What is your evaluation of the potential usefulness of the library?
Very useful, as it makes the current signal library more generaly usable. Fixing the nasty Qt issue (even though its not boosts fault) is a great maintainace boon, and getting rid of trackable makes it much easier to "sell"
* Did you try to use the library? With what compiler? Did you have any problems?
I use the previous version (thread_safe_signals-2008-03-12) on gcc-4.2 and MSCV-8.0 ( and shortly on 7.1). No problems there, even when using it as drop in replacement for the old signals. I have not encountered any performance problems, but neither did I check for them, and am moderately concerned about signals2 beeing too slow.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
In-depth documentation reading, a quick reading of the source / examples / tests, a bit of usage.
* Are you knowledgeable about the problem domain?
Moderately so, I have used boost::signals before, I used Qt Signals, and maintained a bigger project which used its own signaling mechanism. I'm only moderately knowledgable in threading Issues.
And finally, every review should answer this question:
* Do you think the library should be accepted as a Boost library? Be sure to say this explicitly so that your other comments don't obscure your overall opinion.
I have a tendency to yes, because even now it is better than what we have now (which is not saying that signals1 was bad!), on the other hand I think that the missing documentation is crucial. Regards Fabio
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Monday 10 November 2008 05:30 am, Fabio Fracassi wrote:
* Automatic connection management: This is great, even if it wasn't required for thread safety reasons, I prefer using shared_ptr based connection tracking over boost::trackable, since it enables me to track (and thus safely use) 3d party classes.
One issue that has occurred to me with the shared_ptr use, is it requires the use of boost::shared_ptr. So, if someone's program is instead always using std::shared_ptr for example, that could be a bit of an annoyance. One solution would be to make it support anything that provides the shared_ptr/weak_ptr interface by making slotN::track() a template and doing some type erasure inside the library. Unfortunately, neither the shared_ptr or weak_ptr interface provides a typedef for it's associated weak_ptr/shared_ptr type, and I would need to get both the weak_ptr and shared_ptr type from the single argument passed to slotN::track(). So, I'd have to add some template traits classes that could determine if a class is a shared_ptr or weak_ptr, and the type of its associated weak_ptr or shared_ptr. I could provide specializations of the traits classes for the shared_ptr/weak_ptr implementations I know about (boost, std, tr1, etc), otherwise the user would have to provide a specialization. The other option would be to add yet another template parameter to the signal class for the shared_ptr type, but it has too many parameters already.
I haven't done a comparison, but the Docu seems to be mostly the same as the original signals. Thus for the usecases that are documentated there its high quality, but I miss the same for the new features:
* I miss tutorial quality examples for some of the new features: ** Howto use boost::threads with signals2 ** Howto use postconstructors/predestructors (and what for?) ** An example using signal::extended_slot_type
Yes, it seems clear I'm not going to get away with the minimal porting job I did on the Boost.Signals documentation. Speaking of the postconstructor stuff, I'm not entirely happy with the deconstruct_ptr interface. Maybe I'd like it better if it looked more like make_shared/allocate_shared and had a helper for declaring it as a friend, as has been recently proposed on boost-devel for make/allocate_shared.
* The class reference pages (e.g. doc/html/boost/signals2/connection.html) are missing from the TOC
That's just how boostbook does it. The TOC links to the header synopsis, and the class names in the header synopsis link to the class reference pages. I do remember having a little trouble navigating through it myself initially though.
* Do the portable syntax examples need to be supported (... so prominently)? Maybe they could be moved to an appendix?
I agree they don't, although most of the other additions to the docs people have been asking for would seem to take precedence. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFJGFJu5vihyNWuA4URAsUpAJ0TVggC9K9Zy76p1AxtfnxA1VomsgCeL1Z8 TrXkG8+WbKkBDS7SiUF+0+Y= =zb8l -----END PGP SIGNATURE-----
----- Original Message ----- From: "Fabio Fracassi" <f.fracassi@gmx.net> To: <boost-users@lists.boost.org> Cc: <boost@lists.boost.org> Sent: Monday, November 10, 2008 11:30 AM Subject: Re: [Boost-users] [signals2][review] The review of the signals2 library (formerly thread_safe_signals) begins today, Nov 1st
I have a tendency to yes, because even now it is better than what we have now (which is not saying that signals1 was bad!), on the other hand I think that the missing documentation is crucial.
I think that a mini review would ensure that the documentation is updated as we expect. Vicente
Here is a late review.
* What is your evaluation of the design?
Excellent. The design of Boost.signals is excellent, and Boost.signals2 follows the same design. Boost.signals2 should provide an optional (non-thread safe) compatibility mode with Boost.signals. This mode should not be available by default, but only if the user defines some macro. This will make porting from Boost.signals to Boost.signals2 much smoother. Frank also mentioned the possibility of adding a thread safe version of boost::trackable based on boost::enable_shared_from_this. That would be a very valuable addition to the library.
* What is your evaluation of the implementation?
Have not looked at the implementation.
* What is your evaluation of the documentation?
I agree with the other reviewers that it could be improved.
* What is your evaluation of the potential usefulness of the library?
Very useful. The lack of thread safety is the main weakness of Boost.signals, and is the only point where Boost.signals is inferior to the Qt signals.
* Did you try to use the library? With what compiler? Did you have any problems?
Tried with MSVS 9.0. No problems.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Read the docs. Wrote, compiled and ran a few simple examples.
* Are you knowledgeable about the problem domain?
Yes. I do all my work in an event-driven framework. The three Boost libraries I use most are smart pointers, bind and signals. I also have a lot of experience with the Qt signals.
* Do you think the library should be accepted as a Boost library?
Yes. We need thread safe signals. Many thanks to Frank for submitting this excellent library, Johan Råde
Hi, I don't have a review... I have made a few tests with the library. I think the library is good, but I don't know if I make something wrong, but it seems for me that the library is slow. I have made a few tests with VC++ 8.0 and _SCL_SECURE=0 and it seems that it is about 50 times slower than a normal boost::function with a bind.... It's a lot... Best regards Hansjörg The test code was: class PerformanceCounter { private: LARGE_INTEGER CurrentTickCount_; LARGE_INTEGER OldTickCount_; LARGE_INTEGER TickFrquency_; double EllapsedTime_; double TotalTime_; public: PerformanceCounter(): EllapsedTime_(0.0), TotalTime_(0.0) { ::QueryPerformanceFrequency(&TickFrquency_); ::QueryPerformanceCounter(&CurrentTickCount_); OldTickCount_ = CurrentTickCount_; } void Reset(const double intial_time = 0.0) { TotalTime_ = intial_time; EllapsedTime_ = intial_time; ::QueryPerformanceCounter(&CurrentTickCount_); OldTickCount_ = CurrentTickCount_; } void CalculateEllapsedTime() { ::QueryPerformanceCounter(&CurrentTickCount_); EllapsedTime_ = (double)(CurrentTickCount_.QuadPart - OldTickCount_.QuadPart)/TickFrquency_.QuadPart; TotalTime_ += EllapsedTime_; OldTickCount_ = CurrentTickCount_; } void CalculateEllapsedTime(const PerformanceCounter& to_intialize) { CurrentTickCount_ = to_intialize.CurrentTickCount(); EllapsedTime_ = (double)(CurrentTickCount_.QuadPart - OldTickCount_.QuadPart)/TickFrquency_.QuadPart; TotalTime_ += EllapsedTime_; OldTickCount_ = CurrentTickCount_; } const LARGE_INTEGER& CurrentTickCount() const {return CurrentTickCount_;} double EllapsedMilliseconds() const {return EllapsedTime_ * 1000;} double EllapsedSeconds() const {return EllapsedTime_;} double TotalTime() const {return TotalTime_;} }; template <typename ParamT> class EventHandlerBase1 { public: virtual void notify(ParamT param) = 0; }; template <typename ListenerT,typename ParamT> class EventHandler1 : public EventHandlerBase1<ParamT> { typedef void ( ListenerT::*PtrMember)(ParamT); ListenerT* m_object; PtrMember m_member; public: EventHandler1(ListenerT* object, PtrMember member) : m_object(object), m_member(member) {} void notify(ParamT param) { (m_object->*m_member)(param); } }; template <typename ParamT> class EventHandlerFunc1 : public EventHandlerBase1<ParamT> { typedef void ( * HandlerFunc)(ParamT); HandlerFunc m_handler; public: EventHandlerFunc1(HandlerFunc f) : m_handler(f) {} void notify(ParamT p) { m_handler(p); } }; template <typename ParamT> class EventHandlerStdCallFunc1 : public EventHandlerBase1<ParamT> { typedef void (_stdcall * HandlerFunc)(ParamT); HandlerFunc m_handler; public: EventHandlerStdCallFunc1(HandlerFunc f) : m_handler(f) {} void notify(ParamT p) { m_handler(p); } }; /// <summary> /// C++ event with one parameter /// </summary> template <typename ParamT> class CppEvent1 { typedef std::map< int, EventHandlerBase1<ParamT> *> HandlersMap; HandlersMap m_handlers; int m_count; public: CppEvent1() : m_count(0) {} ~CppEvent1() { HandlersMap::iterator it = m_handlers.begin(); for(; it != m_handlers.end(); it++) { delete it->second; } } /// <summary> /// Attach a member function to the event /// </summary> template <typename ListenerT> CppEventHandler attach(ListenerT* object,void (ListenerT::*member)(ParamT)) { typedef void (ListenerT::*PtrMember)(ParamT); m_handlers[m_count]=new EventHandler1<ListenerT,ParamT>(object,member); m_count++; return m_count-1; } typedef void ( * Callback)(ParamT); /// <summary> /// Attach a static function to the event /// </summary> CppEventHandler attach(Callback f) { m_handlers[m_count]=new EventHandlerFunc1<ParamT>(f); m_count++; return m_count-1; } typedef void (_stdcall * MngCallback)(ParamT); /// <summary> /// Attach a managed function to the event /// </summary> CppEventHandler attach(MngCallback f) { m_handlers[m_count]=new EventHandlerStdCallFunc1<ParamT>(f); m_count++; return m_count-1; } /// <summary> /// Detach an event handler from the event /// </summary> bool detach(CppEventHandler id) { HandlersMap::iterator it = m_handlers.find(id); if(it == m_handlers.end()) return false; delete it->second; m_handlers.erase(it); return true; } /// <summary> /// Fire the event /// </summary> void notify(ParamT param) { HandlersMap::iterator it = m_handlers.begin(); for(; it != m_handlers.end(); it++) { it->second->notify(param); } return; } }; CppEvent1< char > cppEvent; class TestCppEvent { public: CppEventHandler eventHandler; int counter; void Open() { eventHandler = cppEvent.attach(this,&TestCppEvent::OnEventReceived); counter=0; } void Close() { cppEvent.detach(eventHandler); } void OnEventReceived(char b) { counter++; } }; class TestBind { public: int counter; TestBind():counter(0){} void OnEventReceived(char b) { counter++; } }; class TestSignal { public: int counter; TestSignal():counter(0){} void OnEventReceived(char b) { counter++; } }; int _tmain(int argc, _TCHAR* argv[]) { PerformanceCounter timer; TestCppEvent testCppEvent; testCppEvent.Open(); timer.Reset(); for(int i = 0; i < 10000000; i++) { cppEvent.notify('a'); } timer.CalculateEllapsedTime(); testCppEvent.Close(); cout<<"Zeit[TestCppEvent]:"<<timer.EllapsedMilliseconds()<<endl; cout<<testCppEvent.counter << endl; TestBind testBind; boost::function<void (char)> f = boost::bind(&TestBind::OnEventReceived,&testBind,_1); timer.Reset(); for(int i = 0; i < 10000000; i++) { f('a'); } timer.CalculateEllapsedTime(); cout<<"Zeit[TestBind]:"<<timer.EllapsedMilliseconds()<<endl; cout<<testBind.counter << endl; TestSignal testSignal; boost::signals2::signal<void (char)> sig; sig.connect(boost::bind(&TestSignal::OnEventReceived,&testSignal,_1));; timer.Reset(); for(int i = 0; i < 10000000; i++) { sig('a'); } timer.CalculateEllapsedTime(); cout<<"Zeit[TestSignal]:"<<timer.EllapsedMilliseconds()<<endl; cout << testSignal.counter << endl; } Johan Råde schrieb:
Here is a late review.
* What is your evaluation of the design?
Excellent. The design of Boost.signals is excellent, and Boost.signals2 follows the same design.
Boost.signals2 should provide an optional (non-thread safe) compatibility mode with Boost.signals. This mode should not be available by default, but only if the user defines some macro. This will make porting from Boost.signals to Boost.signals2 much smoother.
Frank also mentioned the possibility of adding a thread safe version of boost::trackable based on boost::enable_shared_from_this. That would be a very valuable addition to the library.
* What is your evaluation of the implementation?
Have not looked at the implementation.
* What is your evaluation of the documentation?
I agree with the other reviewers that it could be improved.
* What is your evaluation of the potential usefulness of the library?
Very useful. The lack of thread safety is the main weakness of Boost.signals, and is the only point where Boost.signals is inferior to the Qt signals.
* Did you try to use the library? With what compiler? Did you have any problems?
Tried with MSVS 9.0. No problems.
* How much effort did you put into your evaluation? A glance? A quick reading? In-depth study?
Read the docs. Wrote, compiled and ran a few simple examples.
* Are you knowledgeable about the problem domain?
Yes. I do all my work in an event-driven framework. The three Boost libraries I use most are smart pointers, bind and signals. I also have a lot of experience with the Qt signals.
* Do you think the library should be accepted as a Boost library?
Yes. We need thread safe signals.
Many thanks to Frank for submitting this excellent library, Johan Råde
On Wednesday 19 November 2008 16:59, Hansi wrote:
I have made a few tests with the library. I think the library is good, but I don't know if I make something wrong, but it seems for me that the library is slow. I have made a few tests with VC++ 8.0 and _SCL_SECURE=0 and it seems that it is about 50 times slower than a normal boost::function with a bind.... It's a lot...
I distilled your benchmark down to something minimal I could compile under Linux (attached) and I get: $ ./a.out 0.25 s 10000000 2.24 s when compiling with gcc with -O3. So, I'm getting a factor of 9. The original Boost.Signals doesn't do any better in this particular benchmark. One odd thing I noticed is the signals2 benchmark actually runs slower (2.8 sec) if I define NDEBUG!
Johan Råde wrote:
Frank also mentioned the possibility of adding a thread safe version of boost::trackable based on boost::enable_shared_from_this. That would be a very valuable addition to the library.
Second that! While I love the generality of the explicit tracking, I've been jumping through some hoops trying to adapt existing Boost.Signals code that uses boost::trackable. If signals2 could provide a thread-safe version of boost::trackable, it would make conversion a much easier "sell" within my organization.
On Friday 16 January 2009 16:35, Nat Goodspeed wrote:
Johan Råde wrote:
Frank also mentioned the possibility of adding a thread safe version of boost::trackable based on boost::enable_shared_from_this. That would be a very valuable addition to the library.
Second that! While I love the generality of the explicit tracking, I've been jumping through some hoops trying to adapt existing Boost.Signals code that uses boost::trackable.
If signals2 could provide a thread-safe version of boost::trackable, it would make conversion a much easier "sell" within my organization.
I have added a thread-UNsafe boost::signals2::trackable to svn to ease porting of single-threaded Boost.Signals code that doesn't need to be made thread-safe. Would you give a little more detail on what parts of boost::trackable porting you've found the most painful? Because to get thread-safe connection management in any form you'd still have to make your objects owned by shared_ptr and deal with the "no shared_from_this() in constructors" issue, since I don't think the release version of enable_shared_from_this supports use in constructors yet. With respect to the "no shared_from_this() in constructors" issue, I've added a "deconstruct" factory function based on make_shared and Michael Marcin's make_shared_access patch. It constructs the shared_ptr with a single allocation like make_shared and calls postconstructible::postconstruct like deconstruct_ptr. Its use can be forced by making your constructors private and declaring the class boost::signals2::deconstruct_access a friend.
Sorry for the long delay... I got behind on the Boost list and am only gradually catching up. :-(
Johan Råde wrote:
Frank also mentioned the possibility of adding a thread safe version of boost::trackable based on boost::enable_shared_from_this.
Frank Mori Hess wrote:
I have added a thread-UNsafe boost::signals2::trackable to svn to ease porting of single-threaded Boost.Signals code that doesn't need to be made thread-safe.
That will be useful transitionally if we can count on a thread-safe boost::signals2::trackable arriving later. If signals2::trackable would only ever be thread-unsafe, it's not useful to us. I recently introduced into our code some machinery based on boost::signal. Others are concurrently working on multithreading. I don't want my new functionality to become a source of difficult race bugs -- or even to be /perceived/ that way. I want to avoid a label of: "this mechanism is thread-unsafe, avoid it in all new code." Accordingly, I've just replaced boost::signal with boost::signals2::signal, and so forth, throughout our code. There were several existing uses of boost::trackable. I've coded around them in several ways, as described below. Since I didn't introduce them, I don't know the author's intent.
Would you give a little more detail on what parts of boost::trackable porting you've found the most painful?
Those of you uninterested in details can skip the rest. :-) My Holler class contains a boost::signals2::signal<void(const Data&)>. For simple usage ignoring lifespan issues (e.g. connecting a free function), I have a Holler::listen(const slot_type&) method. It's nice that with boost::trackable, the same listen() method Just Works. With an instance 'smartptr' of ListenerClass derived from boost::trackable, I'd like to write something like: holler.listen(boost::bind(&ListenerClass::method, smartptr, _1)); If my listen() method were able to tease apart the object returned from bind(), it could detect the case of a shared_ptr<boost::trackable subclass>. Since I don't know how to do that, I've introduced a number of alternative ways to get disconnect-on-destruction. I don't yet know which, if any of them, will become the prevalent idiom. None is as easy/foolproof as boost::trackable, since each requires the caller to explicitly request connection management. Maybe I'm overlooking something that would make my life much easier -- suggestions welcome! 1. I've introduced a template Holler::listen() overload like this: template <class CLASS, typename POINTEE> connection listen(void (CLASS::*method)(const Data&), const boost::shared_ptr<POINTEE>& pointer); Thus, instead of writing: holler.listen(boost::bind(&SomeClass::method, ptr, _1)); you'd write: holler.listen(&SomeClass::method, ptr); This method instantiates a slot_type object, passes 'pointer' to its track() method and then calls the other listen() overload: slot_type listener(boost::bind(method, pointer.get(), _1)); // n.b. gcc 3.3 doesn't like listener(method, pointer.get(), _1) listener.track(pointer); return listen(listener); 2. For transient objects not managed by shared_ptr, I've also introduced a Trackable base class containing boost::ptr_vector<boost::signals2::scoped_connection> mConnections; The Trackable::track(const connection& c) method does this: mConnections.push_back(new scoped_connection(c)); So destroying a Trackable subclass object disconnects any connections passed to track(). Again, if my Holler::listen() method could detect a bind() involving a TrackableSubclass*, it could implicitly call Trackable::track(). 2a. You can explicitly engage track() using syntax such as: listenobj.track(holler.listen(boost::bind(&TrackableSubclass::method, listenobj, _1))); Trackable also defines a suite of listenTo() methods: 2b. connection listenTo(Holler&, const slot_type&); 2c. // harmonious with the Holler::listen() overload template <class CLASS, typename POINTER> connection listenTo(Holler&, void (CLASS::*method)(const Data&), const POINTER& pointer); // doesn't need to be boost::shared_ptr because we're // using Trackable::track() rather than slot_type::track() 2d. // for a Trackable subclass object to bind one of its own methods template <class CLASS> connection listenTo(Holler&, void (CLASS::*method)(const Data&)); 3. One of my colleagues has a class that (with my changes) now also wraps a boost::signals2::signal. He strongly dislikes the variation between my listen(const slot_type&) and listen(method ptr, shared_ptr) methods, so he's changed his own connectFor() method as follows: template <typename POINTER> connection connectFor(const slot_type& slot, const POINTER& ptr) { connection c(mSignal.connect(slot)); Trackable* isTrackable(dynamic_cast<Trackable*>(ptr)); if (isTrackable) { isTrackable->track(c); } return c; } (Yes, this could be made cleverer to notice when POINTER is actually a boost::shared_ptr<SOMETHING>. Next iteration.) So you subscribe to his class with a uniform call such as: obj.connectFor(boost::bind(&SomeClass::method, aPointer, _1), aPointer); But then you have to pass a NULL pointer for the case of a free function -- leaving open the possibility that someone will carelessly copy-and-paste the wrong example instance and pass NULL with a boost::bind() expression. As I said above, every one of these approaches requires coding something other than the intuitive listen(boost::bind(etc.)) call, meaning you must know when you need to request connection management. The variety of options is undoubtedly a bad thing: how to choose? You must know way more than you should about what I had in mind. A thread-safe boost::signals2::trackable mechanism would allow us to drop back to a single universal call: holler.listen(boost::bind(&SomeClass::method, ptr, _1)); with connection management automatically engaged as appropriate. A transitional thread-unsafe boost::signals2::trackable would be tenable for now... but if boost::signals2::trackable were only ever going to be thread-unsafe, I couldn't encourage people to use it: we'd be responsible for propagating new thread-unsafety through an application to which we're even now adding threads.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Wednesday 11 February 2009 15:08 pm, Nat Goodspeed wrote:
Frank Mori Hess wrote:
I have added a thread-UNsafe boost::signals2::trackable to svn to ease porting of single-threaded Boost.Signals code that doesn't need to be made thread-safe.
That will be useful transitionally if we can count on a thread-safe boost::signals2::trackable arriving later. If signals2::trackable would only ever be thread-unsafe, it's not useful to us.
It's intended for people who don't want to bother rewriting a lot of Boost.Signals code that doesn't need thread-safety.
With an instance 'smartptr' of ListenerClass derived from boost::trackable, I'd like to write something like:
holler.listen(boost::bind(&ListenerClass::method, smartptr, _1));
If my listen() method were able to tease apart the object returned from bind(), it could detect the case of a shared_ptr<boost::trackable subclass>.
It could, by using boost::visit_each. boost::bind supports visit_each to let you apply a visitor to all the objects bound inside the returned functor. The signals libraries use visit_each to discover trackable objects that have been bound into slots.
Since I don't know how to do that, I've introduced a number of alternative ways to get disconnect-on-destruction. I don't yet know which, if any of them, will become the prevalent idiom. None is as easy/foolproof as boost::trackable, since each requires the caller to explicitly request connection management.
Maybe I'm overlooking something that would make my life much easier -- suggestions welcome!
I would spend some time learning how to use visit_each with bind. You could implement a wrapper function that inspects slots for your own Trackable base class objects, and then takes whatever step is needed to extract a shared_ptr and pass it to slot::track before connecting.
2. For transient objects not managed by shared_ptr, I've also introduced a Trackable base class containing boost::ptr_vector<boost::signals2::scoped_connection> mConnections;
The Trackable::track(const connection& c) method does this: mConnections.push_back(new scoped_connection(c));
So destroying a Trackable subclass object disconnects any connections passed to track().
Just a note: you need to be careful about connecting these transient objects, since the signal can't employ shared_ptr to prevent them from destructing in mid-signal invocation.
3. One of my colleagues has a class that (with my changes) now also wraps a boost::signals2::signal. He strongly dislikes the variation between my listen(const slot_type&) and listen(method ptr, shared_ptr) methods, so he's changed his own connectFor() method as follows:
template <typename POINTER> connection connectFor(const slot_type& slot, const POINTER& ptr) { connection c(mSignal.connect(slot)); Trackable* isTrackable(dynamic_cast<Trackable*>(ptr)); if (isTrackable) { isTrackable->track(c); } return c; }
Again, with this usage the signal can't prevent destruction in mid-signal invocation.
A thread-safe boost::signals2::trackable mechanism would allow us to drop back to a single universal call:
holler.listen(boost::bind(&SomeClass::method, ptr, _1));
with connection management automatically engaged as appropriate.
You can implement this now, once you get some experience with visit_each. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.6 (GNU/Linux) iD8DBQFJk0CD5vihyNWuA4URAoyIAJ9u1cmheH4+MQx9hhLIZzGg41+x2gCg3Bat 24TIJ9OQCAAyQUXdr+90sqo= =JPSL -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
If my listen() method were able to tease apart the object returned from bind(), it could detect the case of a shared_ptr<boost::trackable subclass>.
It could, by using boost::visit_each. boost::bind supports visit_each to let you apply a visitor to all the objects bound inside the returned functor. The signals libraries use visit_each to discover trackable objects that have been bound into slots.
I would spend some time learning how to use visit_each with bind. You could implement a wrapper function that inspects slots for your own Trackable base class objects, and then takes whatever step is needed to extract a shared_ptr and pass it to slot::track before connecting.
Thanks for the suggestion! I'm excited about this approach, but I've run into a snag. As you note above, in addition to the case of my own Trackable base class, I want my visitor to detect a bound boost::shared_ptr<anything> and pass the shared_ptr to my new slot_type object's track() method. In fact, for a boost::shared_ptr<SomeTrackableSubclass>, I want the shared_ptr tracking to take precedence. As you said, this is the safer mechanism. I can in fact detect a bound shared_ptr and pass it to track() as I want. The problem is that binding a shared_ptr captures a copy, so the referenced object will live until the connection is explicitly disconnected! That makes the slot_type::track() mechanism moot. It looks as though I could only achieve what I want if my visit_each() visitor could *modify* the boost::bind object to replace the bound shared_ptr with its wrapped plain pointer. I don't believe this is possible? (Aside: I tried extending my visitor's detection to boost::weak_ptr, changing my test code by explicitly converting my shared_ptr to weak_ptr before passing it to boost::bind(). I couldn't instantiate slot_type(boost::bind(...boost::weak_ptr<anything>...)) until I specialized boost::get_pointer() myself. (I'm using Boost 1.34.1 augmented with the Signals2 library.) In any case, though, requiring coders to remember to explicitly wrap any boost::smart_ptr instance in boost::weak_ptr feels like the wrong solution.) Perhaps I could build a sufficiently smart boost::visit_each() visitor to reconstruct the original boost::bind() expression, but substituting a plain pointer for any smart_ptr? I'm not confident I can do that without relying on too many boost::bind implementation details. I'm hoping one of you will suggest a better approach. :-)
Just a note: you need to be careful about connecting these transient objects, since the signal can't employ shared_ptr to prevent them from destructing in mid-signal invocation.
Thanks for the warning. I'd prefer to handle bound shared_ptrs, if I can do so without making the referenced objects immortal.
Nat Goodspeed wrote:
I want my visitor to detect a bound boost::shared_ptr<anything> and pass the shared_ptr to my new slot_type object's track() method.
Frank, that functionality isn't specific to either boost::trackable or my own Trackable base class. Supposing we can resolve the problem of a bound copy of a shared_ptr (previous message in thread), couldn't the Signals2 library incorporate that logic itself?
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 17 February 2009, Nat Goodspeed wrote:
Nat Goodspeed wrote:
I want my visitor to detect a bound boost::shared_ptr<anything> and pass the shared_ptr to my new slot_type object's track() method.
Frank, that functionality isn't specific to either boost::trackable or my own Trackable base class. Supposing we can resolve the problem of a bound copy of a shared_ptr (previous message in thread), couldn't the Signals2 library incorporate that logic itself?
I don't view the "dynamically replace bound shared_ptr with weak_ptr in incoming bind functor" idea as viable. But if you mean the general feature of searching for some base class during connect and automatically tracking those objects, that might get added in the future. I would do it strictly as an optional extension though, in seperate headers and only using the library's existing public interfaces. It would add a free function findable by ADL that would look for the trackable base class when connecting, something like: class shared_trackable: public enable_shared_from_this<shared_trackable> {}; namespace bs2 = boost::signals2; template<typename Signal, typename Func> bs2::connection connect(Signal &sig, const Func &f) { typename Signal::slot_type myslot(f); // apply visitor to f here which finds shared_trackable objects // and tracks their owning shared_ptr... shared_trackable_visitor visitor(myslot); visit_each(visitor, f); return sig.connect(myslot); } -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEARECAAYFAkmcHFQACgkQ5vihyNWuA4XvTACgl0tpUa0XYnvWjJaDCS1HBPn+ tJMAoITCZWlj/046/bklVcnTDgN8q0gi =usvO -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
Nat Goodspeed wrote:
I want my visitor to detect a bound boost::shared_ptr<anything> and pass the shared_ptr to my new slot_type object's track() method.
Frank, couldn't the Signals2 library incorporate that logic itself?
I don't view the "dynamically replace bound shared_ptr with weak_ptr in incoming bind functor" idea as viable.
Sigh, yeah -- that would seem to call for a whole new boost::bind() feature supporting copy-with-transformation.
But if you mean the general feature of searching for some base class during connect and automatically tracking those objects, that might get added in the future. I would do it strictly as an optional extension though, in separate headers and only using the library's existing public interfaces. It would add a free function findable by ADL that would look for the trackable base class when connecting, something like:
class shared_trackable: public enable_shared_from_this<shared_trackable> {};
If I understand correctly, we expect the coder to pass to boost::bind() a plain pointer, a reference or a weak_ptr to a shared_trackable subclass instance. None of those will artificially prolong the life of that instance. However, using enable_shared_from_this lets your visitor obtain a shared_ptr to pass to slot_type::track(). Yes? It's still the case that if the coder passes to boost::bind() an actual shared_ptr to the shared_trackable subclass instance, the slot_type::track() mechanism becomes irrelevant because the object won't die. That might warrant a note in the documentation for this feature.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Wednesday 18 February 2009, Nat Goodspeed wrote:
Frank Mori Hess wrote:
But if you mean the general feature of searching for some base class during connect and automatically tracking those objects, that might get added in the future. I would do it strictly as an optional extension though, in separate headers and only using the library's existing public interfaces. It would add a free function findable by ADL that would look for the trackable base class when connecting, something like:
class shared_trackable: public enable_shared_from_this<shared_trackable> {};
If I understand correctly, we expect the coder to pass to boost::bind() a plain pointer, a reference or a weak_ptr to a shared_trackable subclass instance. None of those will artificially prolong the life of that instance. However, using enable_shared_from_this lets your visitor obtain a shared_ptr to pass to slot_type::track(). Yes?
Yes
It's still the case that if the coder passes to boost::bind() an actual shared_ptr to the shared_trackable subclass instance, the slot_type::track() mechanism becomes irrelevant because the object won't die. That might warrant a note in the documentation for this feature.
Yes, that's why I suggested you could have the visitor give a compile error on finding a shared_ptr<shared_trackable> in the bind functor. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEARECAAYFAkmdcTUACgkQ5vihyNWuA4WD6wCgn9OC6qi4QLLfGPa2xSArHzTf NmsAniN+eywOOJvKvyumqj7b3z1Wj2VJ =G27o -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
On Wednesday 18 February 2009, Nat Goodspeed wrote:
It's still the case that if the coder passes to boost::bind() an actual shared_ptr to the shared_trackable subclass instance, the slot_type::track() mechanism becomes irrelevant because the object won't die. That might warrant a note in the documentation for this feature.
Yes, that's why I suggested you could have the visitor give a compile error on finding a shared_ptr<shared_trackable> in the bind functor.
I see. In fact, that compile error could be what prompts our coders to convert to weak_ptr when passing a shared_ptr to boost::bind(). Then there's the interesting question as to whether my visitor should complain about *any* shared_ptr in the boost::bind() object, since any such object becomes effectively immortal. If my visitor handles weak_ptr<anything> by passing the corresponding shared_ptr<anything> to slot_type::track(), then it seems reasonable to me to force our coders to convert any shared_ptr to weak_ptr before boost::bind()ing it. Does that make sense? In that case I don't really understand why my visitor would need to distinguish between shared_ptr<shared_trackable> and shared_ptr<anything_else> -- or weak_ptr<shared_trackable> and weak_ptr<anything_else>. Put differently, shared_trackable only seems to me to make a difference when the visitor encounters a plain pointer or reference to it. In that case, the visitor can use shared_trackable::shared_from_this() and still pass a shared_ptr to slot_type::track(). Am I overlooking something?
On Thursday 19 February 2009, Nat Goodspeed wrote:
Then there's the interesting question as to whether my visitor should complain about *any* shared_ptr in the boost::bind() object, since any such object becomes effectively immortal.
If my visitor handles weak_ptr<anything> by passing the corresponding shared_ptr<anything> to slot_type::track(), then it seems reasonable to me to force our coders to convert any shared_ptr to weak_ptr before boost::bind()ing it. Does that make sense?
In that case I don't really understand why my visitor would need to distinguish between shared_ptr<shared_trackable> and shared_ptr<anything_else> -- or weak_ptr<shared_trackable> and weak_ptr<anything_else>.
Put differently, shared_trackable only seems to me to make a difference when the visitor encounters a plain pointer or reference to it. In that case, the visitor can use shared_trackable::shared_from_this() and still pass a shared_ptr to slot_type::track(). Am I overlooking something?
There is nothing inherently wrong with binding a shared_ptr to a slot. I've done it intentionally myself before. It doesn't necessarily make anything immortal. It's only a problem when the shared_ptr bound to the slot owns an object "A", and then slot is connected to a signal which also lives somewhere inside "A". Then you have the usual shared_ptr-cycle problem, which can be broken by using a weak_ptr and slot::track. But you may be binding a shared_ptr which owns an object "B" which is totally unrelated to object containing the signal you are connecting to. Or, even if objects "A" and "B" are the same type, the objects might for example be arranged in some kind of tree where a shared_ptr owning child object "A" is bound to a slot connected to a signal inside a parent object "B".
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 17 February 2009, Nat Goodspeed wrote:
As you note above, in addition to the case of my own Trackable base class, I want my visitor to detect a bound boost::shared_ptr<anything> and pass the shared_ptr to my new slot_type object's track() method. In fact, for a boost::shared_ptr<SomeTrackableSubclass>, I want the shared_ptr tracking to take precedence. As you said, this is the safer mechanism.
I can in fact detect a bound shared_ptr and pass it to track() as I want. The problem is that binding a shared_ptr captures a copy, so the referenced object will live until the connection is explicitly disconnected! That makes the slot_type::track() mechanism moot.
It looks as though I could only achieve what I want if my visit_each() visitor could *modify* the boost::bind object to replace the bound shared_ptr with its wrapped plain pointer. I don't believe this is possible?
Wouldn't it be better just to make your visitor just detect a bound shared_ptr to Trackable and report it, forcing the calling code to get it right? You could use BOOST_STATIC_ASSERT to give a compile-time error, for example. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEARECAAYFAkmcE6wACgkQ5vihyNWuA4UmrACffunK2sMbWBY8PBC6XPoC4jpL 74oAnio/0JJRAGBE6V7qCwJBLUbjpfQO =HKR1 -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
On Tuesday 17 February 2009, Nat Goodspeed wrote:
I can in fact detect a bound shared_ptr and pass it to track() as I want. The problem is that binding a shared_ptr captures a copy, so the referenced object will live until the connection is explicitly disconnected! That makes the slot_type::track() mechanism moot.
It looks as though I could only achieve what I want if my visit_each() visitor could *modify* the boost::bind object to replace the bound shared_ptr with its wrapped plain pointer. I don't believe this is possible?
Wouldn't it be better just to make your visitor just detect a bound shared_ptr to Trackable and report it, forcing the calling code to get it right? You could use BOOST_STATIC_ASSERT to give a compile-time error, for example.
I'm sorry, I didn't express myself clearly. There are actually several different cases: 1. visitor discovers plain pointer/reference to a subclass of my Trackable base class: Use Trackable to track the new connection. This is working now -- but as you point out, it's less safe than your slot_type::track() mechanism since a Trackable might be destroyed during a slot call. 2. visitor discovers a shared_ptr to something other than a Trackable subclass: This is the case I was asking about. My visitor can in fact pass the shared_ptr to slot_type::track(), but it's pointless because the shared_ptr copy stored in the boost::bind() result makes the referenced object effectively immortal. To my surprise, I find that it's not destroyed even when I explicitly disconnect the resulting connection. 3. visitor discovers a weak_ptr to something other than a Trackable subclass: Though I had to specialize boost::get_pointer() for weak_ptr myself, with that in place, this case works now. My problem is that I can't imagine every coder consistently remembering to convert a shared_ptr to weak_ptr before passing it to boost::bind(). 4. visitor discovers a shared_ptr to a Trackable subclass: This is the case you mention above. To tell you the truth, though, this doesn't look like an error to me. In effect, I have a choice between two different connection-management mechanisms. Given that slot_type::track() is safer than my Trackable mechanism, I'd prefer to use that -- but unless we can somehow create a new boost::bind object that omits the bound shared_ptr, it's moot. 5. visitor discovers a weak_ptr to a Trackable subclass: I currently use the slot_type::track() mechanism for this case too. In other words, once the visitor discovers a shared_ptr or weak_ptr, it stops caring about the pointee type.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Wednesday 18 February 2009, Nat Goodspeed wrote:
1. visitor discovers plain pointer/reference to a subclass of my Trackable base class:
Use Trackable to track the new connection.
This is working now -- but as you point out, it's less safe than your slot_type::track() mechanism since a Trackable might be destroyed during a slot call.
There's also the fact that disconnect doesn't get called until the base class destructor (after the derived class destructors have already run). Really, you might as well be using boost::signals2::trackable for this case, since it is equivalent to what you are doing (although I'd avoid this case entirely if you care about thread-safety).
2. visitor discovers a shared_ptr to something other than a Trackable subclass:
This is the case I was asking about. My visitor can in fact pass the shared_ptr to slot_type::track(), but it's pointless because the shared_ptr copy stored in the boost::bind() result makes the referenced object effectively immortal.
To my surprise, I find that it's not destroyed even when I explicitly disconnect the resulting connection.
It will get destroyed eventually. The signal cleans up its slot list little by little during connect/invoke. It doesn't immediately remove disconnected slots from the slot list since other threads might be using the same slot list concurrently. It might be possible to make it immediately reset the shared_ptr owning the slot though, leaving an empty shared_ptr in the slot list, since that wouldn't invalidate any iterators. -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (GNU/Linux) iEYEARECAAYFAkmdc8oACgkQ5vihyNWuA4U8VwCgvK+rErMUtP458AKqcUhiKUPF 55oAn0GKKGMVKqc2C1CF1VkBE725XcSy =bDoE -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
On Wednesday 18 February 2009, Nat Goodspeed wrote:
1. visitor discovers plain pointer/reference to a subclass of my Trackable base class:
Use Trackable to track the new connection.
This is working now -- but as you point out, it's less safe than your slot_type::track() mechanism since a Trackable might be destroyed during a slot call.
There's also the fact that disconnect doesn't get called until the base class destructor (after the derived class destructors have already run). Really, you might as well be using boost::signals2::trackable for this case, since it is equivalent to what you are doing
Now that it's been introduced [1], I will, thanks!
(although I'd avoid this case entirely if you care about thread-safety).
I understand and agree. Thanks for explaining the details of the vulnerabilities. This now becomes an awareness problem on our side. We have existing uses of the original Boost.Signals library, and some existing classes derived from the original boost::trackable. I don't yet understand the authors' intent, or the typical lifespans of these classes. Nonetheless I'm trying to switch all such usage over to Signals2 with a minimum of disruption so that, as more threads are introduced, we at least have a way to improve thread-safety. I introduced a Trackable base class only as a drop-in, Signals2-compatible replacement for boost::trackable. Since you've now done that officially, mine becomes entirely moot. At the moment I don't want to have to change over the affected classes to be managed by shared_ptr. If I understand correctly, that's the best way to defend a given class against this particular family of race conditions. We're going to have to tackle those incrementally.
2. visitor discovers a shared_ptr to something other than a Trackable subclass:
the shared_ptr copy stored in the boost::bind() result makes the referenced object effectively immortal.
To my surprise, I find that it's not destroyed even when I explicitly disconnect the resulting connection.
It will get destroyed eventually. The signal cleans up its slot list little by little during connect/invoke. It doesn't immediately remove disconnected slots from the slot list since other threads might be using the same slot list concurrently.
Ah -- thanks for clarifying! I wrote a unit test that monitors the lifespan of the class containing my slot method. (The test class binds a bool&, setting it on construction and clearing it on destruction.) I called connection::disconnect(), then immediately inspected that bool flag. Hence my remark above.
It might be possible to make it immediately reset the shared_ptr owning the slot though, leaving an empty shared_ptr in the slot list, since that wouldn't invalidate any iterators.
I like that idea, on the Principle of Least Astonishment. :-) And by the way -- since I don't seem to have expressed this earlier -- many thanks for your work on Signals2! I really appreciate having a migration path to better thread safety. I fear that some of my earlier messages could be read as complaints, when in reality I'm very enthused about switching over. Now if I only had a way to transform a boost::bind() object, replacing any embedded shared_ptr<Foo> with weak_ptr<Foo>, life would be perfect. ;-) But for now, your suggestion of a compile error should work fine. [1] http://www.boostpro.com/vault/index.php?action=downloadfile&filename=signals2-2009-02-11.zip&directory=thread_safe_signals&
participants (8)
-
Fabio Fracassi -
Frank Mori Hess -
Frank Mori Hess -
Hansi -
Johan Råde -
Nat Goodspeed -
Stjepan Rajko -
vicente.botet