From 276ef7897b3bbd3537a4313d420e2f43022a3840 Mon Sep 17 00:00:00 2001 From: David Lechner Date: Fri, 25 Aug 2023 11:49:22 -0500 Subject: [PATCH 1/3] backends/bluezdbus/manager: normalize adapter/device arg checks This makes sure all methods of the BlueZManager are checking that the adapter or device exists in BlueZ before continuing with the method. Documentation is updated to indicate which functions have this check and which do not. --- CHANGELOG.rst | 1 + bleak/backends/bluezdbus/manager.py | 55 ++++++++++++++++++++++++++--- 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8903d1be..30afd8a4 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -23,6 +23,7 @@ Changed * Scanner backends modified to allow multiple advertisement callbacks. Merged #1367. * Changed default handling of the ``response`` argument in ``BleakClient.write_gatt_char``. Fixes #909. +* Changed `BlueZManager` methods to raise `BleakError` when device is not in BlueZ. Fixed ----- diff --git a/bleak/backends/bluezdbus/manager.py b/bleak/backends/bluezdbus/manager.py index 6c580317..d96e45cd 100644 --- a/bleak/backends/bluezdbus/manager.py +++ b/bleak/backends/bluezdbus/manager.py @@ -170,6 +170,22 @@ def __init__(self): self._condition_callbacks: Set[Callable] = set() self._services_cache: Dict[str, BleakGATTServiceCollection] = {} + def _check_adapter(self, adapter_path: str) -> None: + """ + Raises: + BleakError: if adapter is not present in BlueZ + """ + if adapter_path not in self._properties: + raise BleakError(f"adapter '{adapter_path.split('/')[-1]}' not found") + + def _check_device(self, device_path: str) -> None: + """ + Raises: + BleakError: if device is not present in BlueZ + """ + if device_path not in self._properties: + raise BleakError(f"device '{device_path.split('/')[-1]}' not found") + async def async_init(self): """ Connects to the D-Bus message bus and begins monitoring signals. @@ -326,13 +342,15 @@ async def active_scan( Returns: An async function that is used to stop scanning and remove the filters. + + Raises: + BleakError: if the adapter is not present in BlueZ """ async with self._bus_lock: # If the adapter doesn't exist, then the message calls below would # fail with "method not found". This provides a more informative # error message. - if adapter_path not in self._properties: - raise BleakError(f"adapter '{adapter_path.split('/')[-1]}' not found") + self._check_adapter(adapter_path) callback_and_state = CallbackAndState(advertisement_callback, adapter_path) self._advertisement_callbacks.append(callback_and_state) @@ -432,13 +450,15 @@ async def passive_scan( Returns: An async function that is used to stop scanning and remove the filters. + + Raises: + BleakError: if the adapter is not present in BlueZ """ async with self._bus_lock: # If the adapter doesn't exist, then the message calls below would # fail with "method not found". This provides a more informative # error message. - if adapter_path not in self._properties: - raise BleakError(f"adapter '{adapter_path.split('/')[-1]}' not found") + self._check_adapter(adapter_path) callback_and_state = CallbackAndState(advertisement_callback, adapter_path) self._advertisement_callbacks.append(callback_and_state) @@ -534,7 +554,12 @@ def add_device_watcher( Returns: A device watcher object that acts a token to unregister the watcher. + + Raises: + BleakError: if the device is not present in BlueZ """ + self._check_device(device_path) + watcher = DeviceWatcher( device_path, on_connected_changed, on_characteristic_value_changed ) @@ -571,7 +596,12 @@ async def get_services( Returns: A new :class:`BleakGATTServiceCollection`. + + Raises: + BleakError: if the device is not present in BlueZ """ + self._check_device(device_path) + if use_cached: services = self._services_cache.get(device_path) if services is not None: @@ -644,7 +674,12 @@ def get_device_name(self, device_path: str) -> str: Returns: The current property value. + + Raises: + BleakError: if the device is not present in BlueZ """ + self._check_device(device_path) + return self._properties[device_path][defs.DEVICE_INTERFACE]["Name"] def is_connected(self, device_path: str) -> bool: @@ -655,7 +690,7 @@ def is_connected(self, device_path: str) -> bool: device_path: The D-Bus object path of the device. Returns: - The current property value. + The current property value or ``False`` if the device does not exist in BlueZ. """ try: return self._properties[device_path][defs.DEVICE_INTERFACE]["Connected"] @@ -667,7 +702,12 @@ async def _wait_for_services_discovery(self, device_path: str) -> None: Waits for the device services to be discovered. If a disconnect happens before the completion a BleakError exception is raised. + + Raises: + BleakError: if the device is not present in BlueZ """ + self._check_device(device_path) + services_discovered_wait_task = asyncio.create_task( self._wait_condition(device_path, "ServicesResolved", True) ) @@ -695,7 +735,12 @@ async def _wait_condition( device_path: The D-Bus object path of a Bluetooth device. property_name: The name of the property to test. property_value: A value to compare the current property value to. + + Raises: + BleakError: if the device is not present in BlueZ """ + self._check_device(device_path) + if ( self._properties[device_path][defs.DEVICE_INTERFACE][property_name] == property_value From 9a182ed8cf369055cff032109f66cc018ec94515 Mon Sep 17 00:00:00 2001 From: David Lechner Date: Fri, 25 Aug 2023 12:22:21 -0500 Subject: [PATCH 2/3] backends/bluezdbus/manager: race InterfaceRemoved in _wait_for_services_discovery() This adds a 3rd race condition to _wait_for_services_discovery() to handle the case where an interface is removed from BlueZ without changing the "Connected" property. This can happen, e.g. when we hit "retry due to le-connection-abort-by-local". --- CHANGELOG.rst | 1 + bleak/backends/bluezdbus/manager.py | 77 +++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 30afd8a4..ced4d631 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -32,6 +32,7 @@ Fixed * Fixed typing for ``BaseBleakScanner`` detection callback. * Fixed possible crash in ``_stopped_handler()`` in WinRT backend. Fixes #1330. * Reduced expensive logging in the BlueZ backend. Merged #1376. +* Fixed race condition with ``"InterfaceRemoved"`` when getting services in BlueZ backend. `0.20.2`_ (2023-04-19) ====================== diff --git a/bleak/backends/bluezdbus/manager.py b/bleak/backends/bluezdbus/manager.py index d96e45cd..9350a78b 100644 --- a/bleak/backends/bluezdbus/manager.py +++ b/bleak/backends/bluezdbus/manager.py @@ -7,6 +7,7 @@ """ import asyncio +import contextlib import logging import os from typing import ( @@ -708,22 +709,70 @@ async def _wait_for_services_discovery(self, device_path: str) -> None: """ self._check_device(device_path) - services_discovered_wait_task = asyncio.create_task( - self._wait_condition(device_path, "ServicesResolved", True) - ) - device_disconnected_wait_task = asyncio.create_task( - self._wait_condition(device_path, "Connected", False) - ) - done, pending = await asyncio.wait( - {services_discovered_wait_task, device_disconnected_wait_task}, - return_when=asyncio.FIRST_COMPLETED, - ) + with contextlib.ExitStack() as stack: + services_discovered_wait_task = asyncio.create_task( + self._wait_condition(device_path, "ServicesResolved", True) + ) + stack.callback(services_discovered_wait_task.cancel) + + device_disconnected_wait_task = asyncio.create_task( + self._wait_condition(device_path, "Connected", False) + ) + stack.callback(device_disconnected_wait_task.cancel) + + # in some cases, we can get "InterfaceRemoved" without the + # "Connected" property changing, so we need to race against both + # conditions + device_removed_wait_task = asyncio.create_task( + self._wait_removed(device_path) + ) + stack.callback(device_removed_wait_task.cancel) + + done, _ = await asyncio.wait( + { + services_discovered_wait_task, + device_disconnected_wait_task, + device_removed_wait_task, + }, + return_when=asyncio.FIRST_COMPLETED, + ) + + # check for exceptions + for task in done: + task.result() + + if not done.isdisjoint( + {device_disconnected_wait_task, device_removed_wait_task} + ): + raise BleakError("failed to discover services, device disconnected") + + async def _wait_removed(self, device_path: str) -> None: + """ + Waits for the device interface to be removed. - for p in pending: - p.cancel() + If the device is not present in BlueZ, this returns immediately. - if device_disconnected_wait_task in done: - raise BleakError("failed to discover services, device disconnected") + Args: + device_path: The D-Bus object path of a Bluetooth device. + """ + if device_path not in self._properties: + return + + event = asyncio.Event() + + def callback(_: str): + event.set() + + device_removed_callback_and_state = DeviceRemovedCallbackAndState( + callback, self._properties[device_path][defs.DEVICE_INTERFACE]["Adapter"] + ) + + with contextlib.ExitStack() as stack: + self._device_removed_callbacks.append(device_removed_callback_and_state) + stack.callback( + self._device_removed_callbacks.remove, device_removed_callback_and_state + ) + await event.wait() async def _wait_condition( self, device_path: str, property_name: str, property_value: Any From edfbd9366699b0eecc6f6dda4b5bc01bf4c8c7c3 Mon Sep 17 00:00:00 2001 From: David Lechner Date: Fri, 25 Aug 2023 14:08:04 -0500 Subject: [PATCH 3/3] backends/bluezdbus/manager: add device path to condition callbacks This adds a device path key to the condition callbacks. This will avoid calling callbacks for other devices that are not the subject of the current "PropertiesChanged" signal. --- bleak/backends/bluezdbus/manager.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/bleak/backends/bluezdbus/manager.py b/bleak/backends/bluezdbus/manager.py index 9350a78b..53d5f7ac 100644 --- a/bleak/backends/bluezdbus/manager.py +++ b/bleak/backends/bluezdbus/manager.py @@ -67,6 +67,12 @@ class CallbackAndState(NamedTuple): """ +DevicePropertiesChangedCallback = Callable[[], None] +""" +A callback that is called when the properties of a device change in BlueZ. +""" + + DeviceRemovedCallback = Callable[[str], None] """ A callback that is called when a device is removed from BlueZ. @@ -168,7 +174,7 @@ def __init__(self): self._advertisement_callbacks: List[CallbackAndState] = [] self._device_removed_callbacks: List[DeviceRemovedCallbackAndState] = [] self._device_watchers: Set[DeviceWatcher] = set() - self._condition_callbacks: Set[Callable] = set() + self._condition_callbacks: Dict[str, Set[DevicePropertiesChangedCallback]] = {} self._services_cache: Dict[str, BleakGATTServiceCollection] = {} def _check_adapter(self, adapter_path: str) -> None: @@ -805,13 +811,14 @@ def callback(): ): event.set() - self._condition_callbacks.add(callback) + device_callbacks = self._condition_callbacks.setdefault(device_path, set()) + device_callbacks.add(callback) try: # can be canceled await event.wait() finally: - self._condition_callbacks.remove(callback) + device_callbacks.remove(callback) def _parse_msg(self, message: Message): """ @@ -945,7 +952,9 @@ def _parse_msg(self, message: Message): ) # handle device condition watchers - for condition_callback in self._condition_callbacks: + for condition_callback in self._condition_callbacks.get( + message.path, () + ): condition_callback() # handle device connection change watchers