[signals] Automatic connection management and MT
I'm working on a preliminary solution for an interface-compatible but thread-safe implementation of Signals for my project so that I will be able to switch to Boost.Signals should Doug Gregor find the time to add thread-safety there. While deciding what parts to include and which to leave out, I noticed that the automatic connection management is probably not usable in a multi-threaded context in the current form. Using a base class destructor (signals::trackable) to monitor an observer's lifetime is bound to result in race condition trouble with the order of destruction in a class hierarchy. Has that been discussed somewhere and are there any proposed solutions/alternatives? Regards Timmo Stange
On Tuesday 30 January 2007 22:40 pm, Timmo Stange wrote:
I'm working on a preliminary solution for an interface-compatible but thread-safe implementation of Signals for my project so that I will be able to switch to Boost.Signals should Doug Gregor find the time to add thread-safety there.
While deciding what parts to include and which to leave out, I noticed that the automatic connection management is probably not usable in a multi-threaded context in the current form. Using a base class destructor (signals::trackable) to monitor an observer's lifetime is bound to result in race condition trouble with the order of destruction in a class hierarchy.
Has that been discussed somewhere and are there any proposed solutions/alternatives?
I hit the same problem, I had to manually disconnect at the beginning of my destructors, see http://article.gmane.org/gmane.comp.lib.boost.user/24356 Thread-safe automatic disconnect from non-static member functions seems like it would require some kind of extension to the language itself, like a pre-destructor. I recently did the same thing as you, my signals implementation is attached, if you are interested. The namespaces are different in my code, ::boost is ::EPG and ::boost::signals is ::EPG::signalslib. -- Frank
Frank Mori Hess wrote:
I hit the same problem, I had to manually disconnect at the beginning of my destructors, see
Thanks, I must have missed that post of yours and feel a little stupid about raising the same question again now ;).
Thread-safe automatic disconnect from non-static member functions seems like it would require some kind of extension to the language itself, like a pre-destructor.
There is also the possibility of using a wrapper template instead of a base class to put the tracking functionality to the top of the class hierarchy, but I don't think I particularly like that approach, especially as measures to "finalize" the hierarchy would have to be taken.
I recently did the same thing as you, my signals implementation is attached, if you are interested. The namespaces are different in my code, ::boost is ::EPG and ::boost::signals is ::EPG::signalslib.
Thanks for the code, it looks very promising. I was hoping it could be done with a little less runtime overhead (per slot mutex + polymorphism + shared_ptr), but I have to investigate the available options some more. I have one small suggestion for now: You could perhaps move the connection mutex to the base class to avoid too much per-Signature code repetition (use a private virtual do_disconnect() for forwarding, for example). Regards Timmo Stange
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 01 February 2007 11:24 am, Timmo Stange wrote:
Frank Mori Hess wrote:
Thread-safe automatic disconnect from non-static member functions seems like it would require some kind of extension to the language itself, like a pre-destructor.
There is also the possibility of using a wrapper template instead of a base class to put the tracking functionality to the top of the class hierarchy, but I don't think I particularly like that approach, especially as measures to "finalize" the hierarchy would have to be taken.
One solution is to overload the signal::connect function with another version that accepts an additional weak_ptr<void> argument, which points to the object whose member function is being used as a slot. The signal could try to convert the weak_ptr into a shared_ptr<void> whenever it runs the slot. Then the signal can either clean up the connection if the object is dead, or be sure the object won't die while the slot is running. The disadvantage, of course, is it forces people to use shared_ptrs to manage the lifetimes of objects they want to use thread-safe automatic connection management with. But that seems preferable to offering no means of thread-safe automatic connection management at all.
Thanks for the code, it looks very promising. I was hoping it could be done with a little less runtime overhead (per slot mutex + polymorphism + shared_ptr), but I have to investigate the available options some more.
I imagine it could be. Optimization wasn't high on my priority list while writing it, I was aiming more for correctness, followed by ease of implementation.
I have one small suggestion for now: You could perhaps move the connection mutex to the base class to avoid too much per-Signature code repetition (use a private virtual do_disconnect() for forwarding, for example).
I'm not following what you're suggesting. Would you spell it out in more detail, or even better say it with a patch? - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFwk7B5vihyNWuA4URAqa+AJ9b35Dk01ZrWKpqPxp0uzGp99DcBwCg04UB kIA0X8mCMjrMlfmkYeQ7Ask= =IB4D -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 01 February 2007 15:34 pm, Frank Mori Hess wrote:
I imagine it could be. Optimization wasn't high on my priority list while writing it, I was aiming more for correctness, followed by ease of implementation.
And speaking of correctness, I already found a bug in operator() in epg_signals_template.hpp. The scoped_lock for the connection needs to be unlocked before erasing the disconnected connection from the _connectionBodies list. Otherwise, it might try to unlock a deleted mutex. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFwm8G5vihyNWuA4URAnhWAKC6+wWK+XpyN1Qhcdg8oorwFxKC+QCfdEcg kn3+dOuOkJ9BMCe5X7tdm/s= =zKaC -----END PGP SIGNATURE-----
#include <boost/bind.hpp> #include <boost/thread.hpp> #include <iostream> class BufferMoniter { public: void maitain() { std::cout <<" BufferMoniter" << std::endl; } }; class LocalPeer { public: template <class T> void caller(unsigned int interval,T t) { //do something t(); } void fork() { BufferMoniter *pBuffer=new BufferMoniter( ); boost::thread thrd(caller(500,boost::bind(&BufferMoniter::maitain,pBuffer)) ; // that i really want.a fuctor of caller(500,boost::bind(&BufferMoniter::maitain,pBuffer). thre.join(); } }; //many thanks
hongleij@126.com wrote:
#include <boost/bind.hpp> #include <boost/thread.hpp>
#include <iostream>
class BufferMoniter { public: void maitain() { std::cout <<" BufferMoniter" << std::endl; } }; class LocalPeer { public: template <class T> void caller(unsigned int interval,T t) { //do something t(); } void fork() { BufferMoniter *pBuffer=new BufferMoniter( ); boost::thread thrd(caller(500,boost::bind(&BufferMoniter::maitain,pBuffer)) ; // that i really want.a fuctor of caller(500,boost::bind(&BufferMoniter::maitain,pBuffer).
thre.join(); }
The easiest way is to use boost::function<void()> to hide the type of T. This will also take care of one additional problem with interpreting the inner bind as a nested subexpression. void caller(unsigned int interval, boost::function<void()> const & t) { //do something t(); } void fork() { shared_ptr<BufferMoniter> pBuffer( new BufferMoniter() ); // take care of leak boost::function<void()> f = boost::bind(&BufferMoniter::maitain,pBuffer); boost::thread thrd( boost::bind( caller, 500, f ) ); thrd.join(); }
Frank Mori Hess wrote:
One solution is to overload the signal::connect function with another version that accepts an additional weak_ptr<void> argument, which points to the object whose member function is being used as a slot. The signal could try to convert the weak_ptr into a shared_ptr<void> whenever it runs the slot. Then the signal can either clean up the connection if the object is dead, or be sure the object won't die while the slot is running. The disadvantage, of course, is it forces people to use shared_ptrs to manage the lifetimes of objects they want to use thread-safe automatic connection management with. But that seems preferable to offering no means of thread-safe automatic connection management at all.
I see two problems with that (well, three, but I'll leave performance issues out for now ;)): 1. Cleanup only takes place when the signal is actually called, so it may end up tracking a large amount of pointers in a situation where the connection and observer destruction frequency is much higher than the signal call rate. I'm using it to notify thread local storage owners of threads exiting, for example. You can imagine that this won't work well for a signal that is very likely to be called only once at application shutdown, while an arbitrary amount of slots can be connected and disconnected in the meantime. If I'm not mistaken, your current implementation has the same problem for manual disconnections. 2. Using an overload only allows tracking of a fixed number of objects. You're currently just considering the situation in which the object to which the member function is bound to can be tracked, but as far as I understand it, signals::trackable allows you to monitor every bound argument, including ordinary call parameters. Both problems can be solved, of course, so please just see this as a reminder. The overall solution looks good.
I imagine it could be. Optimization wasn't high on my priority list while writing it, I was aiming more for correctness, followed by ease of implementation.
I agree.
I'm not following what you're suggesting. Would you spell it out in more detail, or even better say it with a patch?
Patch attached. Regards Timmo Stange 46,62c46,47 < void disconnect() < { < boost::mutex::scoped_lock lock(mutex); < do_disconnect(); < } < bool connected() const < { < boost::mutex::scoped_lock lock(mutex); < do_connected(); < } < < private: < virtual void do_disconnect() = 0; < virtual bool do_connected() const = 0; < < // mutex should be locked when slot is called, to prevent race with disconnect() < mutable boost::mutex mutex; ---
virtual void disconnect() = 0; virtual bool connected() const = 0;
73,75c58 < < private: < virtual void do_disconnect() ---
virtual void disconnect()
76a60
boost::mutex::scoped_lock lock(mutex);
79c63 < virtual bool do_connected() const ---
virtual bool connected() const
80a65
boost::mutex::scoped_lock lock(mutex);
82a68
// mutex should be locked when slot is called, to prevent race with disconnect()
83a70
mutable boost::mutex mutex;
Timmo Stange wrote:
Frank Mori Hess wrote:
One solution is to overload the signal::connect function with another version that accepts an additional weak_ptr<void> argument, which points to the object whose member function is being used as a slot. [...] Both problems can be solved, of course, so please just see this as a reminder. The overall solution looks good.
I've thought a little more about it: The original trackable solution isn't a real beauty by itself as it is intrusive to entities which it actually tries to treat as "unaware participants" from the functionality perspective. The trackable objects shouldn't need to know that their lifetime is being monitored by the connection. Your shared_ptr approach supports that notion much better in my opinion, but I would prefer to keep it related to the connection and not the signal and have most of the original flexibility in there. How about using a tool like the function argument reference wrappers to mark the tracked objects? The example for automatic connection management would then look like this: struct NewsMessageArea : public MessageArea { public: // ... void displayNews(const NewsItem& news) const { messageText = news.text(); update(); } }; // ... boost::shared_ptr<NewsMessageArea> newsMessageArea(new NewsMessageArea(/* ... */)); // ... deliverNews.connect(boost::bind(&NewsMessageArea::displayNews, boost::signals::track(newsMessageArea), _1)); The wrapper signals::track would expect a shared_ptr and transport a weak_ptr. The information could be collected with visit_each just like it is done now and the number of trackable objects would only be limited by boost::bind. I think it is acceptable that the client code must maintain a shared_ptr to the tracked instance, as that's expressing rather well what is happening here (even though it doesn't really share the ownership, it shares information about the object).
Patch attached.
Um, looks like I got it wrong. Fixed version attached. Regards Timmo Stange 46,47c46,62 < virtual void disconnect() = 0; < virtual bool connected() const = 0; ---
void disconnect() { boost::mutex::scoped_lock lock(mutex); do_disconnect(); } bool connected() const { boost::mutex::scoped_lock lock(mutex); do_connected(); }
private: virtual void do_disconnect() = 0; virtual bool do_connected() const = 0;
// mutex should be locked when slot is called, to prevent race with disconnect() mutable boost::mutex mutex;
58c73,75 < virtual void disconnect() ---
private: virtual void do_disconnect()
60d76 < boost::mutex::scoped_lock lock(mutex); 63c79 < virtual bool connected() const ---
virtual bool do_connected() const
65d80 < boost::mutex::scoped_lock lock(mutex); 68d82 < // mutex should be locked when slot is called, to prevent race with disconnect() 70d83 < mutable boost::mutex mutex;
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Friday 02 February 2007 06:34 am, Timmo Stange wrote:
deliverNews.connect(boost::bind(&NewsMessageArea::displayNews, boost::signals::track(newsMessageArea), _1));
The wrapper signals::track would expect a shared_ptr and transport a weak_ptr. The information could be collected with visit_each just like it is done now and the number of trackable objects would only be limited by boost::bind. I think it is acceptable that the client code must maintain a shared_ptr to the tracked instance, as that's expressing rather well what is happening here (even though it doesn't really share the ownership, it shares information about the object).
That does seem more elegant, although I need to learn more about how boost::trackable and visit_each work to understand how it would be implemented. Is this something you would be willing to code?
Patch attached.
Um, looks like I got it wrong. Fixed version attached.
Your changes are fine with me. I'm going to use protected virtual functions instead of private though, as I find it less confusing (as in "that actually works?", the c++ faq agrees with me!). Also, connected() is missing a return. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFw1LW5vihyNWuA4URApilAKChS0ALoL5bH3nmpwloyTtpTXQSvwCeMOP6 hZ8h+2eaBcxD+qsBX/v1JyM= =sO52 -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Friday 02 February 2007 06:34 am, Timmo Stange wrote:
The wrapper signals::track would expect a shared_ptr and transport a weak_ptr. The information could be collected with visit_each just like it is done now and the number of trackable objects would only be limited by boost::bind.
Okay, I think I've got it now. The signals::track() wrapper would return some class, call it signals::tracked for now, which is implicitly convertible into a weak_ptr. The code in signal::connect() would use visit_each on the return value from bind to look for any signals::tracked objects inside, and if found store them as weak_ptrs associated with the connection. Then any slots with expired weak_ptrs could be cleaned up when the signal is next emitted, or also in signal::connect() to prevent lots of dead connections from accumulating. If we do cleanup in connect(), then I could get rid of the disconnect callback stuff I added, since it would be mostly redundant.
I think it is acceptable that the client code must maintain a shared_ptr to the tracked instance, as that's expressing rather well what is happening here (even though it doesn't really share the ownership, it shares information about the object).
It does share ownership, at least during the periods when the signal is actually running the slot. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFw69s5vihyNWuA4URAsgsAJwJ/lK1CGjV5ztNr4UD9QVDCU9pTwCbBIVg 6LivJe+uFrX7cOhhigkUnmM= =wmTb -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Friday 02 February 2007 06:34 am, Timmo Stange wrote:
The wrapper signals::track would expect a shared_ptr and transport a weak_ptr. The information could be collected with visit_each just like it is done now and the number of trackable objects would only be limited by boost::bind.
Okay, I think I've got it now. The signals::track() wrapper would return some class, call it signals::tracked for now, which is implicitly convertible into a weak_ptr. The code in signal::connect() would use visit_each on the return value from bind to look for any signals::tracked objects inside, and if found store them as weak_ptrs associated with the connection. Then any slots with expired weak_ptrs could be cleaned up when the signal is next emitted, or also in signal::connect() to prevent lots of dead connections from accumulating.
My old (and reiterated) suggestion was to eliminate all the tracking complexity on the signal side and just make the invocation disconnect a slot when it throws bad_weak_ptr. We can make mem_fn convert a weak_ptr argument to a shared_ptr (by abusing get_pointer for weak_ptr) to cover the typical bind( &X::f, wp, ... ) case. This doesn't address your concern that dead slots are only detected when called, but is it really a problem in practice? The remaining thread-safety problem is what happens when a signal is manipulated while being invoked, and are multiple concurrent invocations allowed. The nontrivial part is to solve it without making the invocation performance suffer horribly, which is what people love to benchmark. :-/
Peter Dimov wrote:
My old (and reiterated) suggestion was to eliminate all the tracking complexity on the signal side and just make the invocation disconnect a slot when it throws bad_weak_ptr. We can make mem_fn convert a weak_ptr argument to a shared_ptr (by abusing get_pointer for weak_ptr) to cover the typical bind( &X::f, wp, ... ) case.
That would effectively mean removing the tracking functionality and introducing an exception handling policy to the signal (which could be extended to other/all types of exceptions, made configurable or parameterizable with a suitable template argument) instead. The downside of it is that it loads off the burden to ensure correct results on the exception safety of the client code (which is usually less well tested than the library code) and additionally makes finding potential problems more difficult, because exceptions are silently swallowed by the signal invocation.
This doesn't address your concern that dead slots are only detected when called, but is it really a problem in practice?
I gave one (perhaps a little exotic) example for a valid use of signals where this particular behaviour would disqualify it. In many other cases it could at least result in unexpectedly high memory usage. Think of a user interface that relays user input from permanent elements (like a menu entry) to numerous and often changing other elements (like list entries). For a menu item that's never triggered by the user, you'd get an ever-growing connection list with mostly dead connections. Boost.Signals already solves this problem sufficiently well, by only delaying the connection removal if the disconnect() call originates from the signal call context and retrying to clean up the entire list for every subsequent disconnect().
The remaining thread-safety problem is what happens when a signal is manipulated while being invoked, and are multiple concurrent invocations allowed. The nontrivial part is to solve it without making the invocation performance suffer horribly, which is what people love to benchmark. :-/
From a user perspective I'd expect invocations to work concurrently, while manipulations would require exclusive access, which shouldn't be too difficult to implement (read_write_mutex). I think the cost of invocations is what should be considered primarily (and not for the benchmark results), which is why I criticized the per-slot locking in Frank's implementation. But I agree with him that optimization is something for later. I saw a suggestion to make the thread-safety optional through a policy template parameter and that sounds fine to me. What about an allocator parameter, btw (as the signal is a slot container)? Regards Timmo Stange
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Friday 02 February 2007 19:12 pm, Timmo Stange wrote:
Boost.Signals already solves this problem sufficiently well, by only delaying the connection removal if the disconnect() call originates from the signal call context
My code will deadlock if the slot tries to disconnect() or otherwise manipulate the signal it is connected to. The present semantics are that when disconnect() returns, the slot is disconnected and furthermore not running. I suppose that could be weakened to "the slot is disconnected and not running in any other thread" by using recursive locks. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFxLWw5vihyNWuA4URAhPDAKCU1IS+rvUx+F4N9F21vxhEuaAddwCgiGZZ XkBsD0INMhFQoAtNoMlDz+4= =j5LE -----END PGP SIGNATURE-----
Frank Mori Hess wrote: [...]
Then any slots with expired weak_ptrs could be cleaned up when the signal is next emitted, or also in signal::connect() to prevent lots of dead connections from accumulating. If we do cleanup in connect(), then I could get rid of the disconnect callback stuff I added, since it would be mostly redundant.
As I mentioned in my reply to Peter Dimov, the current Boost.Signals implementation solves this from the call and disconnection context only. A limited number of dangling connections is not a problem and as this approach only prohibits direct removal of connections from the call context (with the same cleanup of dead connections that you already do), the problem I described does not exist. Is there a special reason you started from scratch instead of using Doug's implementation as a starting point? I know I may come a little late with this, but most of it seems to be reusable for a thread-safe version. Regards Timmo Stange
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Friday 02 February 2007 19:24 pm, Timmo Stange wrote:
Frank Mori Hess wrote:
[...]
Then any slots with expired weak_ptrs could be cleaned up when the signal is next emitted, or also in signal::connect() to prevent lots of dead connections from accumulating. If we do cleanup in connect(), then I could get rid of the disconnect callback stuff I added, since it would be mostly redundant.
As I mentioned in my reply to Peter Dimov, the current Boost.Signals implementation solves this from the call and disconnection context only. A limited number of dangling connections is not a problem and as this approach only prohibits direct removal of connections from the call context (with the same cleanup of dead connections that you already do), the problem I described does not exist.
But what if someone creates a large number of short-lived connections and relies entirely on our new automatic connection management scheme, never explicitly calling disconnect()? If the signal is only rarely invoked, the number of dangling connections with invalid weak_ptrs will accumulate, unless you check for them in connect(). I also lean towards checking in connect instead of disconnect because it makes my implementation simpler. If you cleanup in disconnect(), the connection has to call back into the signal to tell it to clean up. Then you have to take steps to be sure the lifetime of the callback doesn't exceed the lifetime of the signal. There are no such worries when doing cleanup in connect(), since it's all inside the signal and the connection isn't involved. The only disadvantage is the signal tends to store 1 more dead slot if cleanup is not done at disconnect.
Is there a special reason you started from scratch instead of using Doug's implementation as a starting point?
No, I probably had similar reasons as you did when you were considering implementing thread-safe signals as a stop-gap until the real thing became thread-safe. I didn't understand the existing implementation, and my ambition was only to implement the minimum I needed for myself. It was just a sidetrack while putting together a little active object framework I'm working on. I couldn't trust the thread-safety of the code without understanding it, so I figured it would be quicker for me to implement the little core I needed than to learn the whole thing and try to tweak it. And I assumed making the real boost.signals thread-safe was not something that could be done in a few days, otherwise someone else would have already done it.
I know I may come a little late with this, but most of it seems to be reusable for a thread-safe version.
It's not late at all. I'm not committed to basing a full implementation on my code. I thought it would be a useful working prototype for exploring ideas while trying to clarify design issues. Whether it ultimately makes more sense to do the full implementation starting from boost.signals, or to try and graft the missing pieces from boost.signals onto the prototype is an open question. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFxLr35vihyNWuA4URAgTbAKDd3+14M6MCx/hUL9Pu4bo03OVntwCbByME VLJ2uXDqRDPXbSkqnPH3Ym8= =gW7r -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
But what if someone creates a large number of short-lived connections and relies entirely on our new automatic connection management scheme, never explicitly calling disconnect()? If the signal is only rarely invoked, the number of dangling connections with invalid weak_ptrs will accumulate, unless you check for them in connect().
Good point. I generally do not like garbage collection behaviour, but it seems unavoidable here. A specialized smart pointer could allow for direct notification when an object goes out of scope, but that would again have the effect that all of the code has to be aware of the trackability (either through intrusive_ptr and a trackable base class again, or through direct use of a pointer type for exclusive use in the signals system).
It's not late at all. I'm not committed to basing a full implementation on my code. I thought it would be a useful working prototype for exploring ideas while trying to clarify design issues. Whether it ultimately makes more sense to do the full implementation starting from boost.signals, or to try and graft the missing pieces from boost.signals onto the prototype is an open question.
I took Boost.Signals, removed grouping, naming and tracking (which are together responsible for more than half of the code) and added rudimentary threading support with a single recursive_mutex. I didn't want to spam the list with several kB of compressed code, so I put it here: http://www.sepulchrum.com/sources/mt_signals.tar.gz I kept the respective namespaces and hope that doesn't pose a problem for testing. What may be interesting to look at is that it works without dangling connections at all. A disconnected slot is always re- moved from the signal as soon as possible. That also simplifies the overall code, but doesn't solve the problem you describe above. The signal cannot be called concurrently with this design, as I've claimed before would be desirable, but I realized that it is complicated with non-trivial combiners anyway. Regards Timmo Stange
Timmo Stange wrote:
I took Boost.Signals, removed grouping, naming and tracking (which are together responsible for more than half of the code) and added rudimentary threading support with a single recursive_mutex. I didn't want to spam the list with several kB of compressed code, so I put it here: http://www.sepulchrum.com/sources/mt_signals.tar.gz
I've cleaned it up a bit, added some comments and replaced the internal IF template with mpl::if_ (as that was a docu- mented todo). There are a few things I am unsure about in the original implementation: - Why does the signal use the pimpl idiom? I don't see any immediate advantage in it. - Is it safe to assume that only temporary slot_call_iterators will ever be dereferenced with the single_pass_traversal_tag? That's how I understand the docs, but it's crucial to my implementation and I better ask twice ("*i++;", no "*i, ++i;"). - Is it really desirable to completely replace the current implemen- tation with a thread-safe one (parameterizable or not) while it seems clear that the tracking mechanism will definitely be different for each version? I've tested my version with random_signal_system.cpp and was sur- prised it did not crash despite the lack of trackable. The bacon test naturally fails occasionally, as connections that should have been removed by the tracking functionality are miraculously re- vived to point to newly added signals. We will obviously need a full wrapper for the weak_ptr to trackable objects, for the inspection through visit_each to work as expected. I suggest a signals::tracked<function_type> class that wraps the actual weak_ptr and a smart_ptr that may be used to lock() the weak_ptr and reset it through two simple member functions lock() and unlock(). A convenience function for boost::bind with the name signals::track() can be used to instantiate such an object similar to std::make_pair(). The simplest way to interface with those signals::tracked<...> objects would be an abstract base class with virtual lock() and unlock(), if we accept the possible code bloat that may involve (locking a series of weak_ptrs into smart_ptrs for every signal invocation won't win any performance awards anyway). Regards FarCry
Another update: I've added tracking similar to what I described before. It now passes the random_signal_test except for the final connection count check, which may be a result of the different signal and connection lifetime control I had to use. I ran it with 20 initial signals, a deletion probability of 0.5 and 10000 iterations and it never called a slot with a dead tracked weak_ptr, which is what we want. It also cleans up those slots for each connect() call. The tracking functionality is still somewhat ugly and its per- formance is horrible, but I think it's a starting point. The signals::tracked<...> wrapper template currently only offers implicit conversion to a shared_ptr, which won't enable binding a pointer to member function to it, so it probably needs a full smart pointer interface. I've also thought about the use of recursive_mutex. I could be turned into a normal mutex if the implementation would unlock it before each slot call and lock it again after that. Regards Timmo Stange
On Monday 05 February 2007 03:57 am, Timmo Stange wrote:
Another update: I've added tracking similar to what I described before. It now passes the random_signal_test except for the final connection count check, which may be a result of the different signal and connection lifetime control I had to use. I ran it with 20 initial signals, a deletion probability of 0.5 and 10000 iterations and it never called a slot with a dead tracked weak_ptr, which is what we want. It also cleans up those slots for each connect() call.
Is there any reason not to put your code into the boos sandbox cvs? It won't conflict with any of my files, as my file and subdirectory name are different. If you do put it in, it might be useful to check in the unmodified boost.signals files as the initial commit, so the cvs can diff all the way back to the originals.
The tracking functionality is still somewhat ugly and its per- formance is horrible, but I think it's a starting point. The signals::tracked<...> wrapper template currently only offers implicit conversion to a shared_ptr, which won't enable binding a pointer to member function to it, so it probably needs a full smart pointer interface.
Wouldn't it be cleaner (and perhaps more robust) to just have lock() return a shared_ptr<void> and get rid of the unlock() and shared_ptr member variable? I'm going to try playing around with your file a bit in EPG::signal today.
I've also thought about the use of recursive_mutex. I could be turned into a normal mutex if the implementation would unlock it before each slot call and lock it again after that.
Then the invocation of a slot won't be atomic with respect to the slot being disconnected. It seems worthwhile to me that we should guarantee the slot isn't still running (in any other thread) when disconnect() returns. There's also the matter of what happens in a combiner if a slot it hasn't reached yet is disconnected while it is running, which also will affect EPG::signal since it allows disconnect to happen during signal invocation, as long as the particular slot in question is not running. I was planning to make the slot iterator dereference operation throw an exception if the slot has been disconnected. I'm also thinking lock contention could be reduced in EPG::signal by making connect() make a new copy of the slot list when a new connection is added, which would allow connect() to return while an signal invocation is still in progress. Already running invocations would continue to iterate over the old slot list, and later invocations would get the updated one. It could be optimized to only create a new copy if a signal invocation is actually in progress. -- Frank
Frank Mori Hess wrote:
Is there any reason not to put your code into the boos sandbox cvs? It won't conflict with any of my files, as my file and subdirectory name are different. If you do put it in, it might be useful to check in the unmodified boost.signals files as the initial commit, so the cvs can diff all the way back to the originals.
I just didn't want to bother Doug on the Super Bowl weekend ;). I've asked for write access now and will do as you suggest (based on the current CVS - I don't know if there are any relevant differences to 1.33.1, but that's the code I used).
Wouldn't it be cleaner (and perhaps more robust) to just have lock() return a shared_ptr<void> and get rid of the unlock() and shared_ptr member variable? I'm going to try playing around with your file a bit in EPG::signal today.
You need to store the smart_ptrs to an arbitrary amount of tracked objects for the duration of the slot call and I thought it might be more practical to use the tracker (I don't use the client's tracked object) for that instead of creating another scoped con- tainer.
Then the invocation of a slot won't be atomic with respect to the slot being disconnected. It seems worthwhile to me that we should guarantee the slot isn't still running (in any other thread) when disconnect() returns.
I agree. I also realized that this would allow concurrent invocation and I don't think that's expected behaviour.
There's also the matter of what happens in a combiner if a slot it hasn't reached yet is disconnected while it is running, which also will affect EPG::signal since it allows disconnect to happen during signal invocation, as long as the particular slot in question is not running. I was planning to make the slot iterator dereference operation throw an exception if the slot has been disconnected.
I'm not sure I understood this correctly. It should be ok for the client to disconnect slots that already have been called or will be called from an invocation context. That's what you have the ordering for. And it's no problem for the signal either.
I'm also thinking lock contention could be reduced in EPG::signal by making connect() make a new copy of the slot list when a new connection is added, which would allow connect() to return while an signal invocation is still in progress. Already running invocations would continue to iterate over the old slot list, and later invocations would get the updated one. It could be optimized to only create a new copy if a signal invocation is actually in progress.
You'd need a second mutex for that to synchronize concurrent connect() calls in one way or the other. I don't know if that's worth the trouble. Regards Timmo Stange
On Monday 05 February 2007 12:49 pm, Timmo Stange wrote:
Frank Mori Hess wrote:
There's also the matter of what happens in a combiner if a slot it hasn't reached yet is disconnected while it is running, which also will affect EPG::signal since it allows disconnect to happen during signal invocation, as long as the particular slot in question is not running. I was planning to make the slot iterator dereference operation throw an exception if the slot has been disconnected.
I'm not sure I understood this correctly. It should be ok for the client to disconnect slots that already have been called or will be called from an invocation context. That's what you have the ordering for. And it's no problem for the signal either.
I'm probably just exposing my ignorance of the boost.signals implementation. Maybe what I'm worrying about can be hidden inside the slot iterator when I get to that. I've been adapting the signals::track() stuff from mt_signal to thread_safe_signal, and as I was testing I've made an annoying discovery. I often like to set up connections in the constructor of an object which connect signals from member objects to slots which are member functions. However, it seems impossible to get a shared_ptr to "this" while inside the constructor that is useable for tracking. Are there any possible work-arounds besides using a private constructor with a static factory function (a solution which doesn't extend well to derived classes), or doing the connection setup in a member function that has to be called separately after the object is constructed? Is there some way to have signals::tracked initially hold a full shared_ptr, until it is known that the object has had a chance to have its ownership passed to an external shared_ptr? Or maybe a version of shared_from_this() which returns a shared_ptr whose use_count is initially artificially inflated by one, until an external shared_ptr comes along and tries to increment the use count. -- Frank
Frank Mori Hess wrote: [...]
Are there any possible work-arounds besides using a private constructor with a static factory function (a solution which doesn't extend well to derived classes), or doing the connection setup in a member function that has to be called separately after the object is constructed?
Is there some way to have signals::tracked initially hold a full shared_ptr, until it is known that the object has had a chance to have its ownership passed to an external shared_ptr? Or maybe a version of shared_from_this() which returns a shared_ptr whose use_count is initially artificially inflated by one, until an external shared_ptr comes along and tries to increment the use count.
I understand your dilemma, but this can't be solved unless we decide to use something completely different from shared_ptr. The idea is that by connecting a slot to a signal, the lifetime tracking of bound objects is started and as soon as all shared_ptr references go out of scope, the slot is disconnected. Even if we could open up a loophole, you could never start tracking from the constructor itself, as the external shared_ptr reference to your object is only initialized when the constructor returns. You would need a second call to the member function to start tracking, in which case you can just as well set up the connection there. But is that really a problem? If you set up the connection from the constructor, you can also disconnect it in the destructor and don't need automated tracking. On a side note: I don't think my current "tracked" wrapper will allow binding to a pointer to member function. It doesn't have overloads for dereferencing and the implicit conversion to shared_ptr won't help here. Regards Timmo Stange
Frank Mori Hess wrote:
[...]
Are there any possible work-arounds besides using a private constructor with a static factory function (a solution which doesn't extend well to derived classes),
The original code also doesn't extend to derived classes if your program takes advantage of the MT-ness of the signal. There is a window after the connection is made in which another thread can invoke the signal and call the slot, accessing the partially constructed object.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Monday 05 February 2007 19:42 pm, Peter Dimov wrote:
The original code also doesn't extend to derived classes if your program takes advantage of the MT-ness of the signal. There is a window after the connection is made in which another thread can invoke the signal and call the slot, accessing the partially constructed object.
I didn't consider that. If a class contains a subobject which creates its own thread and emits signals, the subobject's signals can't be safely connected to the containing object in its constructor. Boost doesn't provide any nice little post-constructor framework by chance? - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFyJqv5vihyNWuA4URAtvuAJ9k0ojelXsHXqIMWJPEnoJywP7GvwCeN3az edTd8EVontmaBImvVv7yLEY= =7xCn -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Monday 05 February 2007 18:28 pm, Timmo Stange wrote:
But is that really a problem? If you set up the connection from the constructor, you can also disconnect it in the destructor and don't need automated tracking.
Yes, I guess it's no big deal. In fact, the reason the possible solutions were unacceptable to me was that manual connection management was so comparatively easy in this case.
On a side note: I don't think my current "tracked" wrapper will allow binding to a pointer to member function. It doesn't have overloads for dereferencing and the implicit conversion to shared_ptr won't help here.
It turns out it doesn't need to be implicitly convertible to shared_ptr at all, like I imagined before I read the mem_fn docs. It just needs a get_pointer function. I'm currently using a stripped down signals::tracked object that is just a thin wrapper around weak_ptr, with the following get_pointer function: template<typename T> T* get_pointer(const signals::tracked<T> &tracked) {return tracked.lock().get();} I'll commit it in a bit, I've still got some bugs to work out (but it does bind and invoke successfully). - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFyJik5vihyNWuA4URAlaaAKC6LMj8g6sS2rCVGUR89SsOevsd3gCg09Vq P2EX9mXVUzVcXTpTorA46lk= =QjBq -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 06 February 2007 10:02 am, Frank Mori Hess wrote:
On Monday 05 February 2007 18:28 pm, Timmo Stange wrote:
On a side note: I don't think my current "tracked" wrapper will allow binding to a pointer to member function. It doesn't have overloads for dereferencing and the implicit conversion to shared_ptr won't help here.
I'll commit it in a bit, I've still got some bugs to work out (but it does bind and invoke successfully).
Okay, I've checked in tracking support to thread_safe_signal. I've verified that it works binding to member functions. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFyLPt5vihyNWuA4URAjwTAJ0TQ/kmVrsRo1wjhsS+tgPRzzJgAwCg2Ih3 sb+wn1lZD56rOEPoyI3eL1k= =5Gm7 -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
It turns out it doesn't need to be implicitly convertible to shared_ptr at all, like I imagined before I read the mem_fn docs. It just needs a get_pointer function. I'm currently using a stripped down signals::tracked object that is just a thin wrapper around weak_ptr, with the following get_pointer function:
That is good news indeed.
template<typename T> T* get_pointer(const signals::tracked<T> &tracked) {return tracked.lock().get();}
I wouldn't use lock() but an explicit shared_ptr conversion here as you will probably want it to throw bad_weak_ptr when the client code uses a dead reference outside your signals functionality. I also realized that the abstract base class design is thankfully unnecessary as a shared_ptr<void> apparently knows what destructor to call when initialized from the correct pointer type (according to the docs). I'll see if I can find the time to commit sample code tonight. Regards Timmo Stange
On Feb 5, 2007, at 6:28 PM, Timmo Stange wrote:
I understand your dilemma, but this can't be solved unless we decide to use something completely different from shared_ptr. The idea is that by connecting a slot to a signal, the lifetime tracking of bound objects is started and as soon as all shared_ptr references go out of scope, the slot is disconnected. Even if we could open up a loophole, you could never start tracking from the constructor itself, as the external shared_ptr reference to your object is only initialized when the constructor returns. You would need a second call to the member function to start tracking, in which case you can just as well set up the connection there.
Does enable_shared_from_this help at all? Cheers, Doug
Douglas Gregor wrote: [Setting up a signal with tracking support from within a constructor]
Does enable_shared_from_this help at all?
I don't think you can safely use it in a constructor to acquire a shared_ptr to yourself, right? It would go out of scope when the constructor exits and your object will be destroyed. One would have to provide a factory function, which is what Frank wanted to avoid. I have a few questions about your Signals implementation, which you could perhaps answer best: The templated signalN::disconnect allows disconnection by "slot", which the docs say elsewhere is difficult to achieve, because of the problem to compare the target function objects. The Boost Function agrees with this, but mentions that Signals knows "a way around it". Now what the implementation seems to do is to simply call operator== on the target function objects. That frankly leaves me somewhat confused. I understand that it should work when the functions are reference wrapped (which will be always the case when the slot target is a signal itself). We could simply mimic the original implementation, but I sure would feel better if I understood what it is going on. Does storing the combiner in Any really help fighting code bloat as mentioned under "Type Erasure" in the docs? It is only used in the signal template. It's copied and invoked, the former by utilizing the any copy constructor which is templated (resulting in more code for the heap storage, not in less) and the latter by using unsafe_any_cast, which effectively gives you a pointer to the original type and the actual invocation should therefor result in the same code. I would prefer storing the combiner directly to avoid the any-related heap allocations, but I don't have access to a large variety of compilers to check the code size differences. As a more general question, do you think that declaring trackable deprecated and only providing it for backwards-compatibility is a good solution? Regards Timmo Stange
On Feb 6, 2007, at 1:49 PM, Timmo Stange wrote:
I have a few questions about your Signals implementation, which you could perhaps answer best:
The templated signalN::disconnect allows disconnection by "slot", which the docs say elsewhere is difficult to achieve, because of the problem to compare the target function objects. The Boost Function agrees with this, but mentions that Signals knows "a way around it". Now what the implementation seems to do is to simply call operator== on the target function objects. That frankly leaves me somewhat confused. I understand that it should work when the functions are reference wrapped (which will be always the case when the slot target is a signal itself). We could simply mimic the original implementation, but I sure would feel better if I understood what it is going on.
So, the issue with Function's operator== is that it isn't possible to compare two instances of boost::function via the operator==. However, one can compare a boost::function object with a different function object (say, std::plus<int>) using operator==. Signals only relies on the latter behavior, e.g., sig.disconnect(my_func_object); will compare the boost::function stored in each slot against my_func_object using operator==, and remove those that match.
Does storing the combiner in Any really help fighting code bloat as mentioned under "Type Erasure" in the docs? It is only used in the signal template. It's copied and invoked, the former by utilizing the any copy constructor which is templated (resulting in more code for the heap storage, not in less) and the latter by using unsafe_any_cast, which effectively gives you a pointer to the original type and the actual invocation should therefor result in the same code. I would prefer storing the combiner directly to avoid the any-related heap allocations, but I don't have access to a large variety of compilers to check the code size differences.
Storing the combining in an Any was a dumb idea :) Still, you will probably need to keep the combiner on the heap somehow, because if a slot deletes the signal object, you don't want to crash. This could happen in a GUI system, for example, in a "button pressed" event on a "cancel" button, which deletes the dialog holding the "cancel" button.
As a more general question, do you think that declaring trackable deprecated and only providing it for backwards-compatibility is a good solution?
If trackable is the wrong thing to do in an MT-context, then by all means go ahead and deprecate it. Cheers, Doug
Doug Gregor wrote:
So, the issue with Function's operator== is that it isn't possible to compare two instances of boost::function via the operator==. However, one can compare a boost::function object with a different function object (say, std::plus<int>) using operator==. Signals only relies on the latter behavior, e.g.,
sig.disconnect(my_func_object);
will compare the boost::function stored in each slot against my_func_object using operator==, and remove those that match.
Ok, I think I got it now, thanks. So all the signal implementation has to take care of is to keep the my_func_object type as a template.
Storing the combining in an Any was a dumb idea :)
Still, you will probably need to keep the combiner on the heap somehow, because if a slot deletes the signal object, you don't want to crash. This could happen in a GUI system, for example, in a "button pressed" event on a "cancel" button, which deletes the dialog holding the "cancel" button.
For the thread-safe implementation we will also need to keep the signal's mutex alive for that case, so I was thinking of using the impl shared_ptr (although I'm afraid of too many atomic reference count adjustments being involved in every signal invocation). Ensuring the combiner simultaneously would bring us back to square one with storing it in the base class (as an Any eventually). I don't think the current implementation allows deletion of the signal from a slot, btw. The signalN::combiner() function returns a reference to the stored combiner, not a copy of it. How about simply copying it in operator(), as being CopyConstructable is a precondition for the combiner anyway? Thanks for the clarifications. Regards Timmo Stange
On Feb 6, 2007, at 3:09 PM, Timmo Stange wrote:
Doug Gregor wrote:
Storing the combining in an Any was a dumb idea :)
Still, you will probably need to keep the combiner on the heap somehow, because if a slot deletes the signal object, you don't want to crash. This could happen in a GUI system, for example, in a "button pressed" event on a "cancel" button, which deletes the dialog holding the "cancel" button.
For the thread-safe implementation we will also need to keep the signal's mutex alive for that case, so I was thinking of using the impl shared_ptr (although I'm afraid of too many atomic reference count adjustments being involved in every signal invocation). Ensuring the combiner simultaneously would bring us back to square one with storing it in the base class (as an Any eventually).
Not necessarily. One could have a "signal_impl_with_combiner" template that derives from "signal_impl", and stores the combiner. Most of the shared code will still be in signal_impl, of course.
I don't think the current implementation allows deletion of the signal from a slot, btw. The signalN::combiner() function returns a reference to the stored combiner, not a copy of it. How about simply copying it in operator(), as being CopyConstructable is a precondition for the combiner anyway?
When we invoke a signal via operator(), operator() will create an instance of boost::signals::detail::call_notification that will live throughout the invocation. Among other things, this call_notification object holds a shared_ptr to the impl (which contains the combiner). Even if the "signal" object is destroyed, that shared_ptr will keep alive all of the data for the signal (including the combiner) so that the invocation can still return properly. Cheers, Doug
Doug Gregor wrote:
Not necessarily. One could have a "signal_impl_with_combiner" template that derives from "signal_impl", and stores the combiner. Most of the shared code will still be in signal_impl, of course.
Right, I'm going to use that, but: While I was running the first experiments with a version of Signals from which I stripped grouping/naming, I've now had a closer look at named_slot_map. I know that it has been con- sidered a not so lucky design and one can tell from the code that it gave you some headaches. I frankly find the performance aspects unacceptable for my personal use (shared_ptr<void> to a heap allocated group name, boost::function as a comparison operator), so the only option for me seems to be turning the named_slot_map into a template, with the obvious propagation effects for the rest of Signals. I would also add a signals::no_name type to be able to get an instantiation which would use a list or deque for the slots. I know that this will result in the infamous template bloat on some compilers, with code duplication for each Group/GroupCompare pair, potentially in several compilation units. On the other hand I won't be able to find the necessary motivation to work on some- thing I'm never going to use myself. So, if such a design change is a show-stopper, I'd rather be told now than later ;). Thanks for the insight on the call_notification mechanism. Regards Timmo Stange
On Feb 7, 2007, at 2:57 PM, Timmo Stange wrote:
Doug Gregor wrote:
Not necessarily. One could have a "signal_impl_with_combiner" template that derives from "signal_impl", and stores the combiner. Most of the shared code will still be in signal_impl, of course.
Right, I'm going to use that, but: While I was running the first experiments with a version of Signals from which I stripped grouping/naming, I've now had a closer look at named_slot_map. I know that it has been con- sidered a not so lucky design and one can tell from the code that it gave you some headaches. I frankly find the performance aspects unacceptable for my personal use (shared_ptr<void> to a heap allocated group name, boost::function as a comparison operator), so the only option for me seems to be turning the named_slot_map into a template, with the obvious propagation effects for the rest of Signals. I would also add a signals::no_name type to be able to get an instantiation which would use a list or deque for the slots.
I know that this will result in the infamous template bloat on some compilers, with code duplication for each Group/GroupCompare pair, potentially in several compilation units. On the other hand I won't be able to find the necessary motivation to work on some- thing I'm never going to use myself.
So, if such a design change is a show-stopper, I'd rather be told now than later ;).
I would not shed a tear if named slot maps were to disappear, and without the std::map needed for named_slot_map, most of the need for the uses of function/any disappear. Cheers, Doug
Doug Gregor wrote:
I would not shed a tear if named slot maps were to disappear, and without the std::map needed for named_slot_map, most of the need for the uses of function/any disappear.
But I can't really make them go away, since they're part of the template signature and removing Group/GroupCompare would break existing code. What I can do is moving the related implementation to a "legacy" part of the library and select the signal base class from new/legacy depending on the Group parameter. Regards Timmo Stange
On Feb 7, 2007, at 4:04 PM, Timmo Stange wrote:
Doug Gregor wrote:
I would not shed a tear if named slot maps were to disappear, and without the std::map needed for named_slot_map, most of the need for the uses of function/any disappear.
But I can't really make them go away, since they're part of the template signature and removing Group/GroupCompare would break existing code. What I can do is moving the related implementation to a "legacy" part of the library and select the signal base class from new/legacy depending on the Group parameter.
That sounds good. Cheers, Doug
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 06 February 2007 10:23 am, Douglas Gregor wrote:
On Feb 5, 2007, at 6:28 PM, Timmo Stange wrote:
I understand your dilemma, but this can't be solved unless we decide to use something completely different from shared_ptr. The idea is that by connecting a slot to a signal, the lifetime tracking of bound objects is started and as soon as all shared_ptr references go out of scope, the slot is disconnected. Even if we could open up a loophole, you could never start tracking from the constructor itself, as the external shared_ptr reference to your object is only initialized when the constructor returns. You would need a second call to the member function to start tracking, in which case you can just as well set up the connection there.
Does enable_shared_from_this help at all?
You know I think it might, at least using a modified or derived version of it. Apparently, shared_ptrs will call into objects derived from enable_shared_from_this in order to set their _internal_weak_this weak_ptr. If we could hook into that call, we could have it call a virtual function called postconstruct(), and voila! We have a postconstructer where signals can be connected to member functions with tracking, and there is no danger of a slot being called before the object is fully constructed. Subclassing works relatively well too, the subclass just has to be sure to call its base class' postconstruct() call if it overrides it. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFyNYM5vihyNWuA4URApaFAJ9uj68vrc9gcKB7KLtCipu+GSOwvwCfQeR/ msXvI0fBodbvcpZD3K56++4= =t3rZ -----END PGP SIGNATURE-----
On Tuesday 06 February 2007 14:25 pm, Frank Mori Hess wrote:
On Tuesday 06 February 2007 10:23 am, Douglas Gregor wrote:
Does enable_shared_from_this help at all?
You know I think it might, at least using a modified or derived version of it. Apparently, shared_ptrs will call into objects derived from enable_shared_from_this in order to set their _internal_weak_this weak_ptr. If we could hook into that call, we could have it call a virtual function called postconstruct(), and voila! We have a postconstructer where signals can be connected to member functions with tracking, and there is no danger of a slot being called before the object is fully constructed. Subclassing works relatively well too, the subclass just has to be sure to call its base class' postconstruct() call if it overrides it.
I've made some minor changes to shared_ptr.hpp and enable_shared_from_this.hpp as a proof of concept (diffs against boost 1.32 attached). It appears to work beautifully. The "real thing" would also take steps to make sure the postconstructor only gets called once (maybe in a "postconstructible" class derived from enable_shared_from_this. And the virtual function that is called in enable_shared_from_this might want a less specific name than postconstruct. Peter, is there any chance a change like this could get put in shared_ptr? -- Frank
Frank Mori Hess wrote:
I've made some minor changes to shared_ptr.hpp and enable_shared_from_this.hpp as a proof of concept (diffs against boost 1.32 attached). It appears to work beautifully. The "real thing" would also take steps to make sure the postconstructor only gets called once (maybe in a "postconstructible" class derived from enable_shared_from_this. And the virtual function that is called in enable_shared_from_this might want a less specific name than postconstruct.
Peter, is there any chance a change like this could get put in shared_ptr?
I don't think adding polymorphism to something as widely used as enable_shared_from_this is an option. An alternative would be a smart pointer for exclusive use to track bound slot objects. That would also allow direct notification of the slot when the reference count drops to zero. The downside is again (actually worse than with trackable) that the entire client code must be made aware of the tracking. Regards Timmo Stange
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Tuesday 06 February 2007 15:43 pm, Timmo Stange wrote:
Frank Mori Hess wrote:
I've made some minor changes to shared_ptr.hpp and enable_shared_from_this.hpp as a proof of concept (diffs against boost 1.32 attached). It appears to work beautifully. The "real thing" would also take steps to make sure the postconstructor only gets called once (maybe in a "postconstructible" class derived from enable_shared_from_this. And the virtual function that is called in enable_shared_from_this might want a less specific name than postconstruct.
Peter, is there any chance a change like this could get put in shared_ptr?
I don't think adding polymorphism to something as widely used as enable_shared_from_this is an option.
Maybe the equivalent can be done using only with templates without polymorphism, I'll see if I can make that work. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFyO7Y5vihyNWuA4URAqbWAJ96KRrK1SDYussP8fcY9/zjrI/AqQCgoPE4 H3LPRJ4UK828w+T4SgeNg5M= =OUz9 -----END PGP SIGNATURE-----
On Tuesday 06 February 2007 16:10 pm, Frank Mori Hess wrote:
On Tuesday 06 February 2007 15:43 pm, Timmo Stange wrote:
Frank Mori Hess wrote:
I've made some minor changes to shared_ptr.hpp and enable_shared_from_this.hpp as a proof of concept (diffs against boost 1.32 attached). It appears to work beautifully. The "real thing" would also take steps to make sure the postconstructor only gets called once (maybe in a "postconstructible" class derived from enable_shared_from_this. And the virtual function that is called in enable_shared_from_this might want a less specific name than postconstruct.
Peter, is there any chance a change like this could get put in shared_ptr?
I don't think adding polymorphism to something as widely used as enable_shared_from_this is an option.
Maybe the equivalent can be done using only with templates without polymorphism, I'll see if I can make that work.
Okay, here's the partial-template-specialization proof of concept, where polymorphism is confined to a specialization of enable_shared_from_this. -- Frank
Frank Mori Hess wrote:
Okay, here's the partial-template-specialization proof of concept, where polymorphism is confined to a specialization of enable_shared_from_this.
It's a partial template specialization and not all compilers support that well enough. I know that you can work around this and I don't want to be a spoilsport, but I think this is a too intrusive approach for the limited purpose it satisfies. What about exception safety regarding the call to postconstruct, for example? What if a derived class implements postconstruct for its own purposes and doesn't forward it up the class hierarchy? Wouldn't you need a predestruct for this to be usable for more than your specific needs? I think you're better off writing a specialized smart pointer if you want that kind of functionality. Regards Timmo Stange
On Tuesday 06 February 2007 17:48 pm, Timmo Stange wrote:
Frank Mori Hess wrote:
Okay, here's the partial-template-specialization proof of concept, where polymorphism is confined to a specialization of enable_shared_from_this.
It's a partial template specialization and not all compilers support that well enough. I know that you can work around this and I don't want to be a spoilsport, but I think this is a too intrusive approach for the limited purpose it satisfies. What about exception safety regarding the call to postconstruct, for example? What if a derived class implements postconstruct for its own purposes and doesn't forward it up the class hierarchy? Wouldn't you need a predestruct for this to be usable for more than your specific needs?
Actually, I've realized there's no reason it has to be tied so closely to enable_shared_from_this. There could be a completely independent base class called boost::postconstructible which is treated similarly to enable_shared_from_this in shared_ptr.hpp. Similarly, there could be a boost::predestructible base class that gets its hook called before the shared_ptr deletes its pointer (perhaps only in the default deleter). I believe the hooks would compile to nothing if the shared_ptr isn't holding a pointer derived from one of these classes, just like (I assume) they do for enable_shared_from_this.
I think you're better off writing a specialized smart pointer if you want that kind of functionality.
I'm no so keen on a specialized smart pointer, for reasons that you've already brought up earlier. But I suppose that's what I'll do if it comes to it. It shouldn't be too bad to come up with a new name and do a search and replace on shared_ptr.hpp. My point is, enable_shared_from_this was deemed important enough for special hooks to be put into shared_ptr.hpp for it. So there is precedent. And postconstructors/predestructors aren't peculiar ideas that I just came up with myself. I'm sure they could be useful in other contexts. -- Frank
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Monday 05 February 2007 03:57 am, Timmo Stange wrote:
The tracking functionality is still somewhat ugly and its per- formance is horrible, but I think it's a starting point. The signals::tracked<...> wrapper template currently only offers implicit conversion to a shared_ptr, which won't enable binding a pointer to member function to it, so it probably needs a full smart pointer interface.
One thing I noticed, I think you need to declare virtual destructors on your base classes. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFx1BY5vihyNWuA4URAte5AJ492RNz+24zRCXKdGV/qMyCqwKfygCfUktw HWdJL8yfiwL3bkcRjTNMKmA= =AoYq -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
One thing I noticed, I think you need to declare virtual destructors on your base classes.
Yes, thank you, that will be necessary in the tracker_base - certainly not the only bug ;). The tracked_base won't need one as tracked<> objects will never be deleted through a base class pointer. Regards Timmo Stange
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Sunday 04 February 2007 15:36 pm, Timmo Stange wrote:
Timmo Stange wrote: - Is it really desirable to completely replace the current implemen- tation with a thread-safe one (parameterizable or not) while it seems clear that the tracking mechanism will definitely be different for each version?
Maybe the boost::trackable could be removed from the official interface, but still be provided by the implementation for the sake of backwards compatibility? - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFx0cW5vihyNWuA4URAgB3AJ40O5VVpsmiAcLjVgTLAk+fyltf0wCfQQYG CRRUUYr86Wp48/YzIatYcLY= =50gQ -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Saturday 03 February 2007 18:28 pm, Timmo Stange wrote:
I took Boost.Signals, removed grouping, naming and tracking (which are together responsible for more than half of the code) and added rudimentary threading support with a single recursive_mutex. I didn't want to spam the list with several kB of compressed code, so I put it here: http://www.sepulchrum.com/sources/mt_signals.tar.gz
I'm noticing the mutex appears in the precompiled library part of the code. A desirable design goal for the final implementation is for the user to be able to put thread-unsafe signals in his single-threaded code and not have to link to libboost_thread and other threading libraries. It seems like that would either require the mutex code be kept in the headers, in classes templated by the thread-safety policy (probably partial template specializations), or two separate precompiled library provided, one for the thread-safe and one for the thread-unsafe signals. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFx0jy5vihyNWuA4URAhgxAKDI4WWRts5E6PZ4HnuCkokfo8G7TQCgjBME kTOjhJjMGJ7iYf/RhL4n/As= =IwaO -----END PGP SIGNATURE-----
Frank Mori Hess wrote:
I'm noticing the mutex appears in the precompiled library part of the code. A desirable design goal for the final implementation is for the user to be able to put thread-unsafe signals in his single-threaded code and not have to link to libboost_thread and other threading libraries. It seems like that would either require the mutex code be kept in the headers, in classes templated by the thread-safety policy (probably partial template specializations), or two separate precompiled library provided, one for the thread-safe and one for the thread-unsafe signals.
Such a policy template parameter would probably make the whole library header only anyway. Regards Timmo Stange
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 01 February 2007 18:37 pm, Timmo Stange wrote:
1. Cleanup only takes place when the signal is actually called, so it may end up tracking a large amount of pointers in a situation where the connection and observer destruction frequency is much higher than the signal call rate. I'm using it to notify thread local storage owners of threads exiting, for example. You can imagine that this won't work well for a signal that is very likely to be called only once at application shutdown, while an arbitrary amount of slots can be connected and disconnected in the meantime. If I'm not mistaken, your current implementation has the same problem for manual disconnections.
Good point. I'll add another boost::function callback to the connection body that it will call on disconnect, passing a shared pointer to itself as the argument. The callback will run code in the signal to find the pointer in its connection list and erase it, and the connection body would destruct when it returns from disconnect().
2. Using an overload only allows tracking of a fixed number of objects. You're currently just considering the situation in which the object to which the member function is bound to can be tracked, but as far as I understand it, signals::trackable allows you to monitor every bound argument, including ordinary call parameters.
I hadn't considered that. - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFw03b5vihyNWuA4URAjDeAJ9fEdCnmWRbL0KHvSzZ8CAYzk7PUwCgv1fJ IQqCH5UMjAl8qGMv44elQkU= =Obc2 -----END PGP SIGNATURE-----
On Jan 30, 2007, at 10:40 PM, Timmo Stange wrote:
I'm working on a preliminary solution for an interface-compatible but thread-safe implementation of Signals for my project so that I will be able to switch to Boost.Signals should Doug Gregor find the time to add thread-safety there.
It is unlikely that I will ever find the time to do this, despite the fact that it is the most-requested feature in any piece of software I have ever written. I urge you (and Frank, and others that have gone down this path) to consider completing a thread-safe signals/slots library and submitting it to Boost. If you maintain (or nearly maintain) the Signals interface, I would be thrilled make your thread- safe implementation into Signals version 2, completely replacing the existing version. Cheers, Doug
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Thursday 01 February 2007 10:03 am, Doug Gregor wrote:
It is unlikely that I will ever find the time to do this, despite the fact that it is the most-requested feature in any piece of software I have ever written. I urge you (and Frank, and others that have gone down this path) to consider completing a thread-safe signals/slots library and submitting it to Boost. If you maintain (or nearly maintain) the Signals interface, I would be thrilled make your thread- safe implementation into Signals version 2, completely replacing the existing version.
Doug, Any chance we could put the thread-safe signals headers we are working on in the boost sandbox cvs (I notice you are one of the admins)? I've already got a user account on sourceforge, "fmhess". - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFw0ci5vihyNWuA4URAkbTAKDKP9lF9n/SS69aYzZStrw6m94hXwCgrA82 2uwq2gOWegowHiQ61DO2g18= =RprE -----END PGP SIGNATURE-----
On Feb 2, 2007, at 9:13 AM, Frank Mori Hess wrote:
Any chance we could put the thread-safe signals headers we are working on in the boost sandbox cvs (I notice you are one of the admins)?
Please do.
I've already got a user account on sourceforge, "fmhess".
You now have CVS read/write access to the sandbox. If anyone else needs read/write access, drop me an e-mail with your SourceForge user ID. Cheers, Doug
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On Friday 02 February 2007 10:23 am, Douglas Gregor wrote:
On Feb 2, 2007, at 9:13 AM, Frank Mori Hess wrote:
Any chance we could put the thread-safe signals headers we are working on in the boost sandbox cvs (I notice you are one of the admins)?
Please do.
Thanks, I've put the headers at "boost-sandbox/boost/thread_safe_signal.hpp" and in the subdirectory "boost-sandbox/boost/thread_safe_signals". - -- Frank -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.1 (GNU/Linux) iD8DBQFFw7ac5vihyNWuA4URAoDkAKCd57/S5KaMhyem4i3XgRKUvMkM/wCdEYsR PP5ZZ8gamdZNvNJq0Xekh1s= =bOIe -----END PGP SIGNATURE-----
On 1/30/07, Timmo Stange <ts@sepulchrum.com> wrote:
I'm working on a preliminary solution for an interface-compatible but thread-safe implementation of Signals for my project so that I will be able to switch to Boost.Signals should Doug Gregor find the time to add thread-safety there.
While deciding what parts to include and which to leave out, I noticed that the automatic connection management is probably not usable in a multi-threaded context in the current form. Using a base class destructor (signals::trackable) to monitor an observer's lifetime is bound to result in race condition trouble with the order of destruction in a class hierarchy.
Has that been discussed somewhere and are there any proposed solutions/alternatives?
What if the signal held a weak_ptr to the slot-object? Before calling, it turns it into a shared_ptr, and voila, you won't get deleted during the call? Tony
On 2/1/07, Gottlob Frege <gottlobfrege@gmail.com> wrote:
What if the signal held a weak_ptr to the slot-object? Before calling, it turns it into a shared_ptr, and voila, you won't get deleted during the call?
Yeah, like Frank said (more clearly). Didn't see that before sending mine...
Tony
participants (7)
-
Doug Gregor -
Douglas Gregor -
Frank Mori Hess -
Gottlob Frege -
hongleij＠126.com -
Peter Dimov -
Timmo Stange