ev3dev / ev3dev-lang-python

Pure python bindings for ev3dev
MIT License
425 stars 144 forks source link

Device objects not thread safe #704

Open dwalton76 opened 4 years ago

dwalton76 commented 4 years ago

I was having problems with the GryoSensor where sometimes when I would try to read the value I would get an empty string instead of an integer which would cause python to barf. David figured out that this was a threading issue, see https://github.com/ev3dev/ev3dev/issues/1269

I am running this:

System info (from ev3dev-sysinfo)

Image file:         ev3dev-stretch-ev3-generic-2018-12-14
Kernel version:     4.14.117-ev3dev-2.3.4-ev3
Brickman:           0.10.2
BogoMIPS:           148.88
Bluetooth:          
Board:              board0
BOARD_INFO_HW_REV=7
BOARD_INFO_MODEL=LEGO MINDSTORMS EV3
BOARD_INFO_ROM_REV=6
BOARD_INFO_SERIAL_NUM=0016533F8FD1
BOARD_INFO_TYPE=main

A potential fix for this is

diff --git a/ev3dev2/__init__.py b/ev3dev2/__init__.py
index 2d2895e..c76cc4d 100644
--- a/ev3dev2/__init__.py
+++ b/ev3dev2/__init__.py
@@ -50,6 +50,7 @@ import fnmatch
 import re
 import stat
 import errno
+import _thread
 from os.path import abspath

 def get_current_platform():
@@ -152,6 +153,7 @@ class Device(object):
         '_path',
         '_device_index',
         '_attr_cache',
+        '_file_lock',
         'kwargs',
     ]

@@ -187,6 +189,7 @@ class Device(object):
         classpath = abspath(Device.DEVICE_ROOT_PATH + '/' + class_name)
         self.kwargs = kwargs
         self._attr_cache = {}
+        self._file_lock = _thread.allocate_lock()

         def get_index(file):
             match = Device._DEVICE_INDEX.match(file)
@@ -239,20 +242,25 @@ class Device(object):

     def _get_attribute(self, attribute, name):
         """Device attribute getter"""
+        self._file_lock.acquire()
         try:
             if attribute is None:
-                attribute = self._attribute_file_open( name )
+                attribute = self._attribute_file_open(name)
             else:
                 attribute.seek(0)
-            return attribute, attribute.read().strip().decode()
+            result = attribute.read().strip().decode()
+            self._file_lock.release()
+            return attribute, result
         except Exception as ex:
+            self._file_lock.release()
             self._raise_friendly_access_error(ex, name, None)

     def _set_attribute(self, attribute, name, value):
         """Device attribute setter"""
+        self._file_lock.acquire()
         try:
             if attribute is None:
-                attribute = self._attribute_file_open( name )
+                attribute = self._attribute_file_open(name)
             else:
                 attribute.seek(0)

@@ -260,9 +268,12 @@ class Device(object):
                 value = value.encode()
             attribute.write(value)
             attribute.flush()
+            self._file_lock.release()
+            return attribute
+
         except Exception as ex:
+            self._file_lock.release()
             self._raise_friendly_access_error(ex, name, value)
-        return attribute

     def _raise_friendly_access_error(self, driver_error, attribute, value):
         if not isinstance(driver_error, OSError):

but this would need some testing to see what sort of performance hit all of the locking and unlocking would create.