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&