Unprotected callbacks from CAN RX thread

[sveinse]

In my work of porting canopen to asyncio, I stubled across that canopen contains a lot of unprotected thread crossings. By and large all callbacks coming from can through Network.notify() run in another thread. This means that all subscribed callbacks must protect any class members access, otherwise interesting results may arise. Many callback handlers have synchronization mechanisms, but there a still quite a few that does not.

emcy.py:EcmyConsumer.on_emcy()  Callbacks are not protected
network.py:NodeScanner.on_message_received()  self.nodes is not protected
nmt.py:NmtBase.on_command()  self._state is update and not protected
nmt.py:NmtSlave.on_command()  self.update_heartbeat() is called which alters state
profiles/p402.py:BaseNode402.on_TPDOs_update_callback()  self.tpdo_values[] updated from thread
sdo/server.py:SdoServer:on_request()  Modifies lots of variables

I haven't investigated how many of these have an implicit safe design. This issue is mostly for information, but in general race conditions due to missing synchronization protection is sporadic and hard to debug.

[acolomb]

Can you give an example of what problems may arise of this?

AFAIK many operations in Python (e.g. dict modifications) are inherently thread-safe. Each access sees a consistent state either from before another thread's modification, or with it applied atomically. So what do we need to "protect" from?

And what measures would you suggest? Adding mutex locks in many places increases the complexity, potential for deadlocks, and possibly hurts performance. So such a thing must be well thought through.

[sveinse]

As mentioned, I haven't investigated how many of these thread crossings are implicitly safe by design, e.g. write-only from RX thread, read-only from main thread and so on. I found approx twice the amount of thread crossings in total than listed above, so the majority were inherently safe. E.g. using threading.Condition or queue.Queue and similar.

Python itself with the GIL make a lot of the py operations atomic, which is great. However, in general, one typical pitfall might be if one reads a class member attribute into a local variable, mutates it and writes it back. This execution might be interrupted mid flight and the RX thread might get the GIL and write that class attribute. The first code resumes controls writes back the locally cached data, overwriting the new data from the RX thread.

One example might be nmt.py:NmtBase.on_command() and .state(), where both read self._state one or more times and then later write back the result. Each of these two can run in two different threads and as such race to set self._state.

Other example is sdo/server.py:SdoServer.on_request(). This method does a lot of various class mutation from the callback. The question here is how much of it is modified from the main thread. If the main thread does not modify any of the class' attributes, then this code is safe. However, it is hard to get an overview over which function runs in which thread.

Again, I do not have detailed analysis of the logic of these files, so I do not know if any of these are problematic. In all cases thread crossings is a very careful business. My agenda was in the best interest to inform about the find. I'm working on the asyncio port of canopen, and there locking needs to be handled a little bit differently. There is no thread crossing to worry about, but one still need to think about race protection.

[sveinse]

For the sake of cleaning up the issues, and that this issue haven't lead to any investigations or changes, let's just close this issue. We can always reopen it again if it resurfaces as a problem.