Re: [Boost-users] [signals2][review] The review of the signals2 library (formerly thread_safe_signals) begins today, Nov 1st

Subject: Re: [Boost-users] [signals2][review] The review of the signals2 library (formerly thread_safe_signals) begins today, Nov 1st
From: Nat Goodspeed (nat_at_[hidden])
Date: 2009-02-19 13:08:03

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&