Race conditions in _MorselDetectionCallback

[mkoval]

rospy.Subscribers run in a separate thread. This means that you have to use locks when accessing or modifying data in the callback. In this script, this includes:

• self.ROBOT_STATE
• self.bite_world_pose
• self.bite_detected

You should also lock the OpenRAVE environment when reading or writing anything. This includes:

• world_camera = self.robot.GetLinks()[7].GetTransform()
• self.robot.SetDOFValues(self.robot.GetDOFValues())
• self.robot.SetTransform(robot_pose)
• self.robot.SetActiveDOFs(activedofs)

[PyryM]

The perception subscriber will work as written because it modifies bite_world_pose first and then sets bite_detected. So worst case scenario you'll just be delayed a frame because you'll see a frame where there is a valid bite_world_pose but bite_detected is still false.

I also don't see any non-trivial synchronization issues with ROBOT_STATE.

I can't comment about the OpenRAVE because I don't know much about it.

[mkoval]

My understanding is that the only operation guaranteed to be atomic in Python is the evaluation of a single bytecode. I am not even positive if this is guaranteed in the spec: the GIL is an implementation detail of CPython that could theoretically be removed in the future.

In theory, I am fine with omitting a lock if you can show that the Python specification guarantees that these operations are atomic:

• self.ROBOT_STATE!="LOOKING_AT_PLATE"
• self.bite_world_pose = np.dot(world_camera,relative_pose)
• self.bite_detected = True

Even so, I think there still may a race condition with self.ROBOT_STATE. What if self.ROBOT_STATE is changed by the other thread after the check self.ROBOT_STATE!="LOOKING_AT_PLATE, but before self.bite_detected = True?

More importantly, I don't see any reason to trawl through the Python specification for a situation like this. The cost of locking in this context is negligible and guarantees that there isn't a race condition.

[mkoval]

Also, we really do need to lock the environment for any OpenRAVE function calls that are not explicitly listed as thread-safe. From the OpenRAVE documentation:

All environment methods are multi-thread safe, but any other method to kinbodies, robots, controllers, planners, etc are not thread safe! If the documentation does not say multi-thread safe, don’t use the method without locking the environment!

This is not just a theoretical problem. OpenRAVE internally stores transforms as quaternions and, if you don't lock the environment around these functions, you'll often have OpenRAVE crash with an assertion error that a quaternion is not normalized.

[PyryM]

The cost of locking is cluttering up the code with unnecessary locks.

[mkoval]

Can a cite the section in the Python standard that says we don't need to lock here?

I prefer "correct and cluttered" over "wrong."

[PyryM]

In cpython and pypy the operations are atomic. Do we really need to worry about hypothetical exotic python implementations where this isn't the case? If so, this seems to be such a common use case in rospy that we should write some properly pythonic wrappers around this common locking behavior.

[mkoval]

I don't think there is a "common locking behavior." The desired locking behavior depends entirely on what you're doing in the Subscriber callback.

[PyryM]

Almost every subscriber that does anything nontrivial is going to want to update some global state, and if that needs to be locked then we need to find a way to wrap that locking that doesn't involve every programmer manually writing locks like we're back in the eighties.

[mkoval]

I think you're being a bit dramatic. In the basic case, the code changes from:

def subscriber(self, msg):
    self.some_data = msg.data
    # set some other fields

To this:

def subscriber(self, msg):
    with self.lock:
        self.some_data = msg.data
        # set some other fields

We're hardly back in the eighties. :smile:

[cdellin]

Cue typical suggestion for method decorator if this is a common use case (-:

[PyryM]

Suggestion accepted and assigned to @mkoval :-)

[mkoval]

I better get started now. I'll have to dig up my mainframe manual from the 80's to figure out the appropriate implementation. :smile:

[PyryM]

How about this:

from threading import RLock

def synchronized(f):
    """ Synchronization decorator for class methods. """

    def newFunction(*args, **kw):
        self = args[0]
        if not hasattr(self, '_lock'):
            self._lock = RLock()
        with self._lock:
            return f(*args, **kw)
    return newFunction

class Testo:
    @synchronized
    def __init__(self, bla):
        self.asdf = bla

    @synchronized
    def foo(self, bar):
        # will deadlock with Lock,
        # which is why we use RLock
        self.baz(bar)

    @synchronized
    def baz(self, v):
        print(v)