Added a mutex to Skeleton.

[mkoval]

This pull request adds an std::mutex to Skeleton. Most of the methods on Skeleton are not thread safe, so using some kind of mutex is necessary in a multi-threaded application. It's useful to attach the mutex to Skeleton so you don't have to manually pass one around to every function that operates on it.

[mxgrey]

👍

I wonder what we should do about ReferentialSkeleton. The ReferentialSkeleton is allowed to refer to multiple skeletons at once, so we can't just have it piggyback on one Skeleton's mutex. I wonder if it would be possible to have some kind of lockMutex() function that returns a vector of locked mutexes where each mutex corresponds to one Skeleton held by the ReferentialSkeleton. If we do that, then I think we should declare the function in MetaSkeleton and override the Skeleton version so that it just locks the one mutex of the Skeleton. We'd also want to tag the lockMutex() function with a warning saying that its return value must be used or else it will be ineffective.

That said, I have no idea if any of this is viable, or if there would be a better approach.

[jslee02]

If we want to make ReferentialSkeleton thread safe as well, then @mxgrey 's approach sounds reasonable. In that case, we should consider to use recursive mutex because a Skeletons can be held by multiple ReferentialSkeleton and they might want to lock the mutex asynchronously.

Since DART is not fully (actually almost) thread safe, we can leave considering ReferentialSkeleton for future. So beside on ReferentialSkeleton, 👍.

[mkoval]

@jslee02 I don't think it's a good idea to nest locks on multiple ReferentialSkeletons like that. It can easily lead to deadlock if you are not careful to always acquire locks in the same order.

@mxgrey and I discussed this a bit offline and thought that a function like std::lock would be a good solution. This function locks a set of mutexes and, if you consistently use std::lock throughout your application, guarantees that deadlock cannot occur. We could add a getMutexes method to ReferentialSkeleton to support this.

Unfortunately, std::lock requires that you know the number of locks at compile time. Boost has a similar function boost::lock that supports std::mutex and does not have the same limitation. Alternatively, we could write a helper function that sorts the mutexes by address before locking them, which might address some performance concerns about std::lock and potential for live lock.

There aren't any straightforward answers here. I'm in favor of adding the getMutexes method and enforcing that it always returns mutexes in a consistent order (e.g. sorted by address). Then it should be safe to simply lock them in order and avoid the std::lock craziness.

As usual: multi-threading is hard. 😞

[psigen]

@jslee02, @mxgrey:
@mkoval and I were discussing this a bit further and had the following idea:

We could take the logic from the above link that @mkoval noted and apply it within a custom implementation of the "lockable" concept (probably TimedLockable) and have that be returned by ReferentialSkeleton::getMutex() under the same API as Skeleton.

You can still deadlock if someone locks the skeletons manually in a pathological order, but that is going to be a problem for any locking scheme that has to acquire multiple mutexes.