Re: [boost] [signals2][review] The review of the signals2library(formerly thread_safe_signals) begins today, Nov 1st

Subject: Re: [boost] [signals2][review] The review of the signals2library(formerly thread_safe_signals) begins today, Nov 1st
From: Frank Mori Hess (frank.hess_at_[hidden])
Date: 2008-11-06 10:13:31

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On Tuesday 04 November 2008 18:57 pm, vicente.botet wrote:
> There is nothing here that justify a new mutex so the user can ask why
> another mutex. As you has already said on other post this was due to
> performance issues. I was only requesting you to add only this in the
> documentation.

There is a sentence mentioning performance in the description of the 
dummy_mutex class, which is linked to from the signals2::mutex page.  I 
suppose signals2::mutex should also have a little rationale paragraph stating 
why it even exists in signals2 (to avoid requiring linking to 
libboost_thread), and that it will probably be deprecated and turned into a 
typedef to boost::mutex at some time in the future.

> Are you saying that the callbacks are called with the mutex unlocked? 

Yes.

> If 
> this is the case, the external_locking strategy needs the collaboration of
> the Signal2 library to wrap these calls with unlock/lock guard. So I don't
> think I can do it above  signals2[dummy_mutex].

You could have the connect methods of your externally locked mutexes wrap the 
slots in a function that unlocks the external mutex before calling the 
wrapped slot.  You'd have to take some care to avoid your wrapper slot from 
hiding the tracked objects in the original slot, but it could be done.  

That raises a related issue: currently the implementation will automatically 
disconnect a slot that throws an expired_slot exception.  A signals2::slot 
will throw an expired_slot exception if it has expired slots when called.  
This has the nice effect that a slot wrapped inside something else before 
being connected to a signal will still automatically disconnect via throwing 
an expired_slot exception.  On the down side, supporting this feature 
requires combiners to catch and ignore expired_slot exceptions that might be 
thrown by dereferencing the slot iterators.

I removed documentation of this feature because I thought it wasn't needed due 
to the addition of extended_slot_type/connect_extended, which provide another 
convenient way for a slot to disconnect itself without throwing expired_slot.  
But it does handle the case of connection management for slots called 
indirectly by a signal invocation, so maybe I should put it back in.

> This seems to what I was purposing you" The library can define a
> basic_signal template class and two specializations, signal
> (single_threaded) and ts_signal (multi_threaded)."

Yes, it's very similar.  I just wanted to emphasize a separation between the 
signals 2 interface "proper", and another optional interface that would exist 
just to support Boost.Signals compatibility.

> I was suggesting a path to provide backwards compatibility. I you have
> another this is OK for me. I would like only to know if you plan to ensure
> backwards compatibility and have more details on how.

My plan would be to keep Boost.Signals around as long as people deem desirable 
(the specific schedule doesn't matter much to me).  The only change to it 
would be in the documentation, stating it's deprecated in favor of Signals2, 
and it may be removed in the future.

Beyond that, I'd add a compatibility trackable class in the signals2 namespace 
which could be used by people with preexisting single-threaded code using 
boost::trackable, who don't want to convert all their trackable classes to be 
owned by shared_ptr and mess with postconstructors.  The use of the trackable 
compatibility class would be discouraged in the documentation for new code, 
but it would be kept in the Signals2 library permanently. 

> >> The section Q&A: How has the API changed from the original
> >> Boost.Signals? must be placed at the begining of the docummentation and
> >> should include the
> >> best practice on how to migrate to the new version.

> > The above suggestions on improving the documentation generally seem good.
> > I'm
> > willing to make another pass over the docs and try to flesh them out and
> > address the above issues.
>
> Could you be more precise on which issues are you ready to take in account
> and which not?

All of them, except possibly the one about putting the porting section at the 
beginning of the documentation (depending on what you mean by that).  I'll 
put the porting section as a top-level section, so it is visible in the table 
of contents on the front page, but I don't regard it as more important than 
the tutorial or reference sections.

>
> Could you explain why you have abandoned the use of boost::any?

I didn't abandon it, it simply never existed in the Signals2 implementation.  
Signals2 didn't start from the Boost.Signals implementation.  It started as a 
minimal, incomplete, thread-safe implementation of the Boost.Signals 
interface for my own use until the real Boost.Signals library was made 
thread-safe.  See this post and its replies for more info:

http://lists.boost.org/boost-users/2007/01/24995.php

Actually, there is a lot of discussion in february of 2007 on the boost-users 
list where we are discussing the development of this library, and Timmo 
Stange's version (which actually did start from the Boost.Signals 
implementation).

> > The shared_ptr/weak_ptr connection management is used because
> > boost::trackable
> > can't be made thread-safe.  It disconnects in the boost::trackable
> > destructor, but since boost::trackable is a base class, its destructor is
> > not
> > called until the derived class destructor has already run.  Thus, it
> > leaves
> > open the possibility of a signal being invoked and using a partially
> > destructed object.
> >
> > signal::combiner returned a reference, so modification through the
> > returned
> > reference could not be protected by a mutex internal to the signal.
> > Getting/setting the combiner by value at least insure the combiner will
> > always be in a well-defined state when used, although they don't protect
> > against multiple threads doing get/modify/set on a signals combiner
> > concurrently.
>
> Would you add this to the design rationale?

Yes

> Does the random_signal_system.cpp  pass the 1.37 regression? If yes, you
> should try again.

It still seems to have some problem when I try using a fairly recent boost svn 
trunk revision (49360).

When I use the command line options "2 0.5 10", the test hangs maybe 10% of 
the time.  That's entirely using Boost.Signals, not my code.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)

iD8DBQFJEwmf5vihyNWuA4URAhUdAJ9+2wouVW035m3EBDowIWbzX5Z4MgCfXvPF
L6yC+EEM97BmFMtKJb4kbvo=
=IntN
-----END PGP SIGNATURE-----