On Saturday 17 February 2007 02:21 pm, Timmo Stange wrote:

Timmo Stange wrote:

...

The localConnectionBodies is a shared pointer to the actual slot list and not a full copy, so I don't understand how this is going to be thread-safe against concurrent invocations. The mutex is released after this compound statement and the invocation traverses a list that might be modified by the "nolock_cleanup_connections(false);" in a concurrent call to the same function.

Sorry, I missed the use_count() check even though I quoted it ;). So concurrent invocation was a bad example, but what about other concurrent modifications of the slot container, like connect()?

connect also checks use_count() after locking the slot list mutex. It calls nolock_force_unique_connection_list(), which creates a (expensive, I know) new copy of the slot list if it is already in use.

Another thing: The original signal is safe against deletion from a slot invocation context. That's why it uses pimpl-Idiom.

Hmm, I didn't consider that case. Since I've already got the pimpl there for signal tracking, I suppose I just need to create a local copy of the shared_ptr pimpl to prevent it from destructing in mid-invocation. -- Frank

On Saturday 17 February 2007 03:34 pm, Timmo Stange wrote:

Frank Mori Hess wrote:

...

connect also checks use_count() after locking the slot list mutex. It calls nolock_force_unique_connection_list(), which creates a (expensive, I know) new copy of the slot list if it is already in use.

Hm, I found that function, but I don't see it getting called from connect().

I neglected to mention create_new_connection() is in the call chain. -- Frank

On Saturday 17 February 2007 01:56 pm, Timmo Stange wrote:

In the slot_call_iterator template I don't understand why you call lockNextCallable in dereference() and call it twice (instead of just once after incrementing the iterator) in increment(). That function may advance the iterator, which can do you no good in dereference() and is unnecessary there, as far as I can tell.

It's to compensate for the fact that it is not called in the constructor, which I originally tried. If it's not called before the first dereference, then you might try to execute an invalid slot. If it's not called before the first increment, then you might not increment past the first slot. I got rid of the call in the constructor because it killed concurrent invocation, due to the iterators still in scope in operator() holding locks to the first slot. However, since I optimized it to only create temporary iterators when passing them to the combiner, I could probably just put the call back into the constructor.

It also doesn't seem like the slot_call_iterator skips disconnected slots.

It does, blocked returns true for disconnected slots. It passes all the test programs (at least it did the last time I tried them). I occasionally get infinite loops and segfaults in random_signal_system.cpp, but that is due to some bug in the test code (the same thing happens with boost.signals 1.32). -- Frank

On Saturday 17 February 2007 03:26 pm, Timmo Stange wrote:

Frank Mori Hess wrote:

...

It's to compensate for the fact that it is not called in the constructor, which I originally tried. If it's not called before the first dereference, then you might try to execute an invalid slot. If it's not called before the first increment, then you might not increment past the first slot. I got rid of the call in the constructor because it killed concurrent invocation, due to the iterators still in scope in operator() holding locks to the first slot. However, since I optimized it to only create temporary iterators when passing them to the combiner, I could probably just put the call back into the constructor.

I'd say the combiner must always check for an empty range and the comparison of first and last will always ensure a valid position, or?

Yes, you're probably right. I must have got it into my head that the combiner couldn't check for an empty range back when I discovered last_value had undefined behaviour if there were no slots connected. -- Frank

Gottlob Frege wrote:

- do you hold any locks DURING the slot call? That's typically bad, since you have no idea what else is locked or what a slot might do, so deadlocks abound.

Yes, that is a problem both implementations have right now.

- do you copy the slot list before running through the list?

Frank's approach has copy-on-write semantics.

- can I connect/disconnect during my slot call (either disconnect myself, or connect/disconnect other calls? If so, does it happen in this signal, or the next one?)

You can do that in both implementations. The behaviour regarding connect()s from a slot context differs. Disconnections always work for the current call, as long as the slot wasn't already passed.

- as for the 'lock per slot' vs 'lock per signal': The solution I've come up with in the past is basically lock per slot, but more importantly *temporary* lock per slot. ie you create (or take from a pool) locks as needed, and store them in a "slot -> lock" map, so IF another thread also needs the lock for that slot, they check the map first, and obtain the same lock for that slot. Using this, if you are careful, in most cases, you don't actually end up creating the lock at all (as you have all the information necessary to see contention as it is happening, and create the locks as needed. ie you know which slot is being called, and which slot is concurrently being disconnected, and only lock if they are the same - which means don't lock on the slot call, but AFTER the call, see if another thread has come in and is waiting for you to finish. So actually it is not a lock at all, but more of an 'event' (or a 'signal' but that'd be confusing).

I'm not sure I completely understood what you're suggesting here. Trying to avoid the use of the synchronization primitives we have (few as they are with Boost.Threads) will most likely lead to trouble. The lock map you're proposing would need to be synched with a mutex, too, so I don't see the immediate gain. I currently only use a per-signal mutex and it should be possible to release that for the duration of the slot call to avoid the deadlocks you mentioned further above. That doesn't give me the freedom of concurrent slot invocation, but seeing how many problems that entails, I'm not sure it is worth the trouble anymore. Regards Timmo Stang

On Sunday 18 February 2007 12:35 pm, Frank Mori Hess wrote:

That's interesting, I hadn't considered dropping the slot lock before running the slot. It would eliminate the possibility of lock-ordering problems with mutexs in the slot/client code. The slot mutex is really just to protect the disconnect and block operations, so by the time the slot is about to be run all we really need to worry about is that the slot function doesn't get deleted while it is executing. I think I could just drop the slot mutex in the slot iterator dereference operation, after either copying the slot function or using some shared_ptr magic to keep it alive, and all would be well.

It turns out shared_ptr magic was already insuring the slot function would not be deleted, so it was just a 1 line change to drop the slot lock before running the slot (in sandbox cvs now). It is now possible for the same slot to be simultaneously executing in multiple threads, but I don't think that violates our design requirements does it? -- Frank

On Sunday 18 February 2007 01:08 pm, Timmo Stange wrote:

Frank Mori Hess wrote:

...

On Sunday 18 February 2007 12:08 am, Gottlob Frege wrote:

...

- do you hold any locks DURING the slot call? That's typically bad, since you have no idea what else is locked or what a slot might do, so deadlocks abound.

Well, neither implementation will deadlock, except for lock-ordering problems between external mutexes and the ones inside the signals implementation. I've got a list lock and a slot lock, and only the slot lock is held while the slot is executing. I took care that it is safe to do a blocking lock on the list lock while a slot lock is already held, and the slot lock itself is recursive so I don't see any deadlocks.

Consider two slots which will mutually disconnect the other in a situation of concurrent signal invocation. With each thread holding the lock of either slot (because of the invocation), neither would be able to disconnect the other - a deadlock.

I stand corrected, my deadlock detector must need tune-up. Dropping the slot mutex before executing has made this deadlock go away too though. -- Frank

On Sunday 18 February 2007 02:16 pm, Timmo Stange wrote:

Frank Mori Hess wrote:

...

I stand corrected, my deadlock detector must need tune-up. Dropping the slot mutex before executing has made this deadlock go away too though.

Hm, since you do the locking in the slot_call_iterator, isn't it the combiner which actually controls locks on the per-slot mutexes? That may be problematic with respect to iterator copies.

I've just checked in some clean-ups, so the iterator doesn't keep a scoped pointer to a lock anymore. I changed lockNextCallable() so it uses the locking form of blocked() and the iterator doesn't need any explicit locking at all (correct me if I'm wrong). Also, it allowed me to dump that memory pool stuff that was trying to optimize dynamic allocation of scoped locks. -- Frank

On Monday 19 February 2007 01:42 am, Timmo Stange wrote:

What I don't like about the release of all mutexes during the call is that we lose the strong semantics of disconnect() and block(). We cannot guarantee anymore, that a slot won't be called after disconnect() returned. That is a form of thread safety only the library can provide.

We could have the connection body dispense some kind of reference-counted token. The slot iterator would grab a copy of the token when it has iterated onto a slot that it might execute, and destroy it when it iterates past the slot or destructs. disconnect()/block() could check the reference count to decide when it is safe to return, either spinning or waiting on a condition variable in the meantime. Since we only want the disconnect() to wait if the slot is running in a different thread, the token would have to maintain both a global and a thread-specific reference count. disconnect()/block() could return when they are equal. -- Frank

On Monday 19 February 2007 02:55 am, Timmo Stange wrote:

...

We could have the connection body dispense some kind of reference-counted token. The slot iterator would grab a copy of the token when it has iterated onto a slot that it might execute, and destroy it when it iterates past the slot or destructs. disconnect()/block() could check the reference count to decide when it is safe to return, either spinning or waiting on a condition variable in the meantime. Since we only want the disconnect() to wait if the slot is running in a different thread, the token would have to maintain both a global and a thread-specific reference count. disconnect()/block() could return when they are equal.

If you don't let disconnect() return until all calls to the slot are through, you're back to square one with regard to potential deadlocks. Keep in mind that disconnect() may be called (in practice often is) from a slot call context.

Yes, I think what I outlined is essentially amounts to a recursive read-write mutex.

I don't think there is a clean solution to this which covers all problems. It would be possible to have different threading policies for either behavior (one being dead-lock-safe, the other with strong disconnect() guarantees), but that defeats the whole abstraction idea.

Yes, I think the strong disconnect() guarantee will also guarantee the deadlock you outlined earlier with two slots attempting to disconnect each other will always be possible.

Alternatively, disconnect() could return a bool if the disconnected slot is currently running or throw an exception. That could be optional through an argument (disconnect(true)) or a different function (checked_disconnect()). A connection::wait() function could wait for all pending calls to finish, but couldn't be safely called from a signal invocation context.

I like connection::wait() better. a return value from disconnect still wouldn't be able to tell you when the slot is really gone, only that it wasn't gone when disconnect returned. -- Frank

Gottlob Frege wrote:

I think the strong guarantee is much more important. If I say disconnect() I really don't expect calls after that. The deadlock only happens when:

1. signals are being processed in multiple threads and 2. slots are disconnecting each other

1 is not what I was typically thinking wrt threaded signals - I was more thinking about connect/disconnect from multiple threads, but the signal would only ever happen on the same thread. Maybe that's too narrow of a view on my part. 2. slots disconnecting each other is really rare/stupid

So I say you make the *default* disconnect wait, and supply an alternative for the rare cases.

I think avoiding potential deadlocks is equally important to the strong disconnection guarantee. The most prominent aspect of Signals is the decoupling between caller and callee it provides. The signal knows nothing about the code it calls and - through generalized callbacks - the called code doesn't even need to know it is called by a signal. We cannot expect from a user that they will carefully steer around potential deadlock problems as that would defeat most of the advantages of using the system in the first place. It's quite possible a user has no control at all over the slot code, for example. On a side note: I don't know if you've followed the tracking functionality changes we've discussed. In a nutshell, we agreed that shared_ptr/weak_ptr is the most portable, useful solution in a multi-threaded environment. Since this kind of tracking ensures the lifetime of the tracked objects during a slot call, a weak disconnect would not keep users from safely releasing resources associated with a slot - as long as they're tracked.

P.S. what about connect/disconnect 'pairing'? ie is there an assumed (or explicit) contract that every call to connect is matched by a disconnect? If I have a slot that is in a signal, and that SAME signal is triggered in, say, 3 threads, the slot can't just call disconnect() (ie the same code in each thread) - it has to somehow carefully keep track that it is calling disconnect() in 1 thread, and the other 2 threads need to skip the disconnect() call.

Or does that matching guarantee not exist?

I don't think I understood this. You can only connect a slot once - if you connect it twice it will be a separate connection and the slot will be called twice. You can only disconnect it once accordingly, although trying to disconnect it several times won't do any harm. Regards Timmo Stange

On 2/19/07, Timmo Stange wrote:

Gottlob Frege wrote:

On a side note: I don't know if you've followed the tracking functionality changes we've discussed. In a nutshell, we agreed that shared_ptr/weak_ptr is the most portable, useful solution in a multi-threaded environment. Since this kind of tracking ensures the lifetime of the tracked objects during a slot call, a weak disconnect would not keep users from safely releasing resources associated with a slot - as long as they're tracked.

I'm trying to keep up but it's not easy!

...

P.S. what about connect/disconnect 'pairing'? ie is there an assumed (or explicit) contract that every call to connect is matched by a disconnect? If I have a slot that is in a signal, and that SAME signal is triggered in, say, 3 threads, the slot can't just call disconnect() (ie the same code in each thread) - it has to somehow carefully keep track that it is calling disconnect() in 1 thread, and the other 2 threads need to skip the disconnect() call.

Or does that matching guarantee not exist?

I don't think I understood this. You can only connect a slot once - if you connect it twice it will be a separate connection and the slot will be called twice. You can only disconnect it once accordingly, although trying to disconnect it several times won't do any harm.

Ignore me. I was translating in my head between different implementations of the same broadcaster/observer patterns. In the one I was last using, the 'subscribe' and 'unsubscribe' in matched pairs.

Regards

Timmo Stange

_______________________________________________ Boost-users mailing list Boost-users@lists.boost.org http://lists.boost.org/mailman/listinfo.cgi/boost-users

Frank Mori Hess wrote:

...

Alternatively, disconnect() could return a bool if the disconnected slot is currently running or throw an exception. That could be optional through an argument (disconnect(true)) or a different function (checked_disconnect()). A connection::wait() function could wait for all pending calls to finish, but couldn't be safely called from a signal invocation context.

I like connection::wait() better. a return value from disconnect still wouldn't be able to tell you when the slot is really gone, only that it wasn't gone when disconnect returned.

How about an optional callback parameter for disconnect(), which usually will be called immediately after the successful disconnection or, in the rare case of the slot being run on another thread at the time of the disconnection, after it returned (naturally from the other thread's context then)? Not specifying this parameter would imply that the caller has taken the necessary precautions to avoid all possible problems (either by externally synchronizing signal calls or by not relying on strong disconnect semantics). The reason for this idea is simplicity of use. Keeping a close watch on resources you share between threads is everyday work, so in many cases special handling for the concurrent disconnection/invocation will not be necessary. In all other cases (when the user has no access to the called code, for example), the tools we provide should be tailored to the problem: this concurrency situation is rather unlikely and the common case should result in synchronous behavior. My connect::wait() idea would often force the user to run a separate maintenance thread for resource cleanup, in which he performs those waits - a high cost (design- and performace-wise) for something that is unlikely to happen. Regards Timmo Stange

Gottlob Frege wrote:

- one of the original reasons (in my head at least) for blocking on disconnect() is so that you can disconnect() in your destructor (and thus avoid having a dead object being called).

But then you need to deal with the possibility that a call can already be in progress at the time your destructor is invoked. (If you can guarantee that this cannot happen, it seems to me that you can also guarantee that a call isn't started after the disconnect.) So let's assume that you lock the object mutex. There's still the nasty scenario where: ~X locks the object mutex X::f is called by the signal and blocks on the object mutex ~X disconnects and destroys *this, including the object mutex X::f crashes and burns I can't think of a way to avoid the above that doesn't also solve the "call after disconnect" problem.

Peter Dimov wrote:

But then you need to deal with the possibility that a call can already be in progress at the time your destructor is invoked. (If you can guarantee that this cannot happen, it seems to me that you can also guarantee that a call isn't started after the disconnect.) So let's assume that you lock the object mutex. There's still the nasty scenario where:

~X locks the object mutex X::f is called by the signal and blocks on the object mutex ~X disconnects and destroys *this, including the object mutex X::f crashes and burns

I can't think of a way to avoid the above that doesn't also solve the "call after disconnect" problem.

Let me give you a short summary of how Frank and I plan to provide tracking for the thread-safe Signals implementation: The client has to maintain ownership of objects to track through shared_ptr. It informs the signal that it wants those objects to be tracked by wrapping that shared_ptr in a signals::tracked object (with a signals::track() helper function for bind(...) et al.). The signal holds the reference to that object as a weak_ptr and will not call the associated slot if it is expired. Otherwise it will hold a local shared_ptr to that object during the slot call, ensuring its lifetime for the duration. The scenario you're discussing is much better handled using that tracking functionality, especially as it makes manual disconnection completely unnecessary. I think it only needs proper documentation to keep the user from trying stunts like the above - provided they read it... Regards Timmo Stange

On 2/19/07, Peter Dimov wrote:

I know. The question was whether the "no calls after disconnect" guarantee is valuable, and disconnecting in a destructor was given as a motivating example. I was just explaining that the guarantee doesn't buy much since disconnecting in a destructor is still unsafe.

Is it? If we are using weak_ptrs? Tony

Gottlob Frege wrote:

On 2/19/07, Peter Dimov wrote:

...

Gottlob Frege wrote:

...

- one of the original reasons (in my head at least) for blocking on disconnect() is so that you can disconnect() in your destructor (and thus avoid having a dead object being called).

But then you need to deal with the possibility that a call can already be in progress at the time your destructor is invoked. (If you can guarantee that this cannot happen, it seems to me that you can also guarantee that a call isn't started after the disconnect.) So let's assume that you lock the object mutex. There's still the nasty scenario where:

~X locks the object mutex X::f is called by the signal and blocks on the object mutex ~X disconnects and destroys *this, including the object mutex X::f crashes and burns

I can't think of a way to avoid the above that doesn't also solve the "call after disconnect" problem.

using weak_ptrs (I think you've heard of them? :-).

Rings a bell. But using weak_ptr's also makes the "no calls after disconnect" guarantee unnecessary; the signal will take proper care of your object's lifetime by keeping a shared_ptr to it during the call. So why provide it? Seems to me that you can't (easily) take advantage of it safely.

Gottlob Frege wrote:

This is starting to sound promising. I could also imagine that disconnect returns some kind of event that you can choose to wait on or not.

[...]

ie, the signal code, instead of calling the disconnect callback, triggers the event.

Not as generic as a callback, but maybe more efficient?

Leaving aside the lack of a simple event primitive in Boost.Threads, we could easily provide a predefined callback functor in Signals that implements this, but I have the feeling we're running in circles here, because that is exactly what we don't want the client code to do: If two slots use that approach with disconnect, the outcome may be a deadlock - not on the slot mutexes this time, but on the "slot return events" ;). Regards Timmo Stange