hww hidapi usage: try to mitigate some thread-safety issues

related: #6097

electrum/plugin.py

@@ -30,6 +30,8 @@ import threading
 import sys
 from typing import (NamedTuple, Any, Union, TYPE_CHECKING, Optional, Tuple,
                     Dict, Iterable, List, Sequence)
+import concurrent
+from concurrent import futures
 
 from .i18n import _
 from .util import (profiler, DaemonThread, UserCancelled, ThreadJob, UserFacingException)
@@ -321,6 +323,20 @@ class HardwarePluginToScan(NamedTuple):
 PLACEHOLDER_HW_CLIENT_LABELS = {None, "", " "}
 
 
+# hidapi is not thread-safe
+# see https://github.com/signal11/hidapi/issues/205#issuecomment-527654560
+#     https://github.com/libusb/hidapi/issues/45
+#     https://github.com/signal11/hidapi/issues/45#issuecomment-4434598
+#     https://github.com/signal11/hidapi/pull/414#issuecomment-445164238
+# It is not entirely clear to me, exactly what is safe and what isn't, when
+# using multiple threads...
+# For now, we use a dedicated thread to enumerate devices (_hid_executor),
+# and we synchronize all device opens/closes/enumeration (_hid_lock).
+# FIXME there are still probably threading issues with how we use hidapi...
+_hid_executor = None  # type: Optional[concurrent.futures.Executor]
+_hid_lock = threading.Lock()
+
+
 class DeviceMgr(ThreadJob):
     '''Manages hardware clients.  A client communicates over a hardware
     channel with the device.
@@ -367,9 +383,15 @@ class DeviceMgr(ThreadJob):
         # locks: if you need to take multiple ones, acquire them in the order they are defined here!
         self._scan_lock = threading.RLock()
         self.lock = threading.RLock()
+        self.hid_lock = _hid_lock
 
         self.config = config
 
+        global _hid_executor
+        if _hid_executor is None:
+            _hid_executor = concurrent.futures.ThreadPoolExecutor(max_workers=1,
+                                                                  thread_name_prefix='hid_enumerate_thread')
+
     def with_scan_lock(func):
         def func_wrapper(self: 'DeviceMgr', *args, **kwargs):
             with self._scan_lock:
@@ -636,7 +658,15 @@ class DeviceMgr(ThreadJob):
         except ImportError:
             return []
 
-        hid_list = hid.enumerate(0, 0)
+        def hid_enumerate():
+            with self.hid_lock:
+                return hid.enumerate(0, 0)
+
+        hid_list_fut = _hid_executor.submit(hid_enumerate)
+        try:
+            hid_list = hid_list_fut.result()
+        except (concurrent.futures.CancelledError, concurrent.futures.TimeoutError) as e:
+            return []
 
         devices = []
         for d in hid_list:

electrum/plugins/bitbox02/bitbox02.py

@@ -46,7 +46,7 @@ class BitBox02Client(HardwareClientBase):
     # handler is a BitBox02_Handler, importing it would lead to a circular dependency
     def __init__(self, handler: Any, device: Device, config: SimpleConfig, *, plugin: HW_PluginBase):
         HardwareClientBase.__init__(self, plugin=plugin)
-        self.bitbox02_device = None
+        self.bitbox02_device = None  # type: Optional[bitbox02.BitBox02]
         self.handler = handler
         self.device_descriptor = device
         self.config = config
@@ -73,6 +73,7 @@ class BitBox02Client(HardwareClientBase):
         return True
 
     def close(self):
+        with self.device_manager().hid_lock:
             try:
                 self.bitbox02_device.close()
             except:
@@ -91,6 +92,7 @@ class BitBox02Client(HardwareClientBase):
                 res = device_response()
             except:
                 # Close the hid device on exception
+                with self.device_manager().hid_lock:
                     hid_device.close()
                 raise
             finally:
@@ -155,6 +157,7 @@ class BitBox02Client(HardwareClientBase):
                 return set_noise_privkey(privkey)
 
         if self.bitbox02_device is None:
+            with self.device_manager().hid_lock:
                 hid_device = hid.device()
                 hid_device.open_path(self.bitbox_hid_info["path"])
 

electrum/plugins/coldcard/coldcard.py

@@ -72,7 +72,7 @@ class CKCCClient(HardwareClientBase):
             self.dev = ElectrumColdcardDevice(dev_path, encrypt=True)
         else:
             # open the real HID device
-            import hid
+            with self.device_manager().hid_lock:
                 hd = hid.device(path=dev_path)
                 hd.open_path(dev_path)
 
@@ -127,6 +127,7 @@ class CKCCClient(HardwareClientBase):
 
     def close(self):
         # close the HID device (so can be reused)
+        with self.device_manager().hid_lock:
             self.dev.close()
         self.dev = None
 

electrum/plugins/digitalbitbox/digitalbitbox.py

@@ -77,6 +77,7 @@ class DigitalBitbox_Client(HardwareClientBase):
 
     def close(self):
         if self.opened:
+            with self.device_manager().hid_lock:
                 try:
                     self.dbb_hid.close()
                 except:
@@ -681,6 +682,7 @@ class DigitalBitboxPlugin(HW_PluginBase):
 
 
     def get_dbb_device(self, device):
+        with self.device_manager().hid_lock:
             dev = hid.device()
             dev.open_path(device.path)
         return dev

electrum/plugins/hw_wallet/plugin.py

@@ -196,6 +196,9 @@ class HardwareClientBase:
     def __init__(self, *, plugin: 'HW_PluginBase'):
         self.plugin = plugin
 
+    def device_manager(self) -> 'DeviceMgr':
+        return self.plugin.device_manager()
+
     def is_pairable(self) -> bool:
         raise NotImplementedError()
 

electrum/plugins/ledger/ledger.py

@@ -74,6 +74,7 @@ class Ledger_Client(HardwareClientBase):
         return True
 
     def close(self):
+        with self.device_manager().hid_lock:
             self.dongleObject.dongle.close()
 
     def timeout(self, cutoff):
@@ -184,13 +185,13 @@ class Ledger_Client(HardwareClientBase):
             self.segwitSupported = self.nativeSegwitSupported or (firmwareInfo['specialVersion'] == 0x20 and versiontuple(firmware) >= versiontuple(SEGWIT_SUPPORT_SPECIAL))
 
             if not checkFirmware(firmwareInfo):
-                self.dongleObject.dongle.close()
+                self.close()
                 raise UserFacingException(MSG_NEEDS_FW_UPDATE_GENERIC)
             try:
                 self.dongleObject.getOperationMode()
             except BTChipException as e:
                 if (e.sw == 0x6985):
-                    self.dongleObject.dongle.close()
+                    self.close()
                     self.handler.get_setup( )
                     # Acquire the new client on the next run
                 else:
@@ -593,6 +594,7 @@ class LedgerPlugin(HW_PluginBase):
                 ledger = True
             else:
                 return None  # non-compatible interface of a Nano S or Blue
+        with self.device_manager().hid_lock:
             dev = hid.device()
             dev.open_path(device.path)
             dev.set_nonblocking(True)