Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Device objects not thread safe #704

Open
dwalton76 opened this issue Jan 4, 2020 · 0 comments
Open

Device objects not thread safe #704

dwalton76 opened this issue Jan 4, 2020 · 0 comments
Assignees

Comments

@dwalton76
Copy link
Collaborator

dwalton76 commented Jan 4, 2020

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 ev3dev/ev3dev#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant