From d15a7c27caaa888561d3de07a29fc6ca8e425ff9 Mon Sep 17 00:00:00 2001 From: Mike Degatano Date: Tue, 27 Sep 2022 02:33:28 -0400 Subject: [PATCH] Don't disconnect in executor (#3901) * Don't disconnect in executor * DBus disconnect doesn't raise * Separate shutdown and disconnect * pylint fix * noop coroutine and test --- supervisor/dbus/agent/__init__.py | 17 +++++---- supervisor/dbus/interface.py | 14 ++++++++ supervisor/dbus/manager.py | 2 +- supervisor/dbus/network/__init__.py | 19 +++++----- supervisor/dbus/network/connection.py | 22 ++++++------ supervisor/dbus/network/interface.py | 18 +++++----- supervisor/dbus/network/setting/__init__.py | 7 ---- supervisor/dbus/network/wireless.py | 2 +- supervisor/dbus/utils.py | 6 ++++ supervisor/utils/dbus.py | 9 +++-- tests/dbus/test_interface.py | 40 ++++++++++++++------- 11 files changed, 99 insertions(+), 57 deletions(-) diff --git a/supervisor/dbus/agent/__init__.py b/supervisor/dbus/agent/__init__.py index 5ac2fe0b1..0b326d863 100644 --- a/supervisor/dbus/agent/__init__.py +++ b/supervisor/dbus/agent/__init__.py @@ -106,10 +106,13 @@ class OSAgent(DBusInterfaceProxy): if not changed and self.datadisk.is_connected: await self.datadisk.update() - def disconnect(self) -> None: - """Disconnect from D-Bus.""" - self.cgroup.disconnect() - self.apparmor.disconnect() - self.system.disconnect() - self.datadisk.disconnect() - super().disconnect() + def shutdown(self) -> None: + """Shutdown the object and disconnect from D-Bus. + + This method is irreversible. + """ + self.cgroup.shutdown() + self.apparmor.shutdown() + self.system.shutdown() + self.datadisk.shutdown() + super().shutdown() diff --git a/supervisor/dbus/interface.py b/supervisor/dbus/interface.py index 678134ce0..910c760c5 100644 --- a/supervisor/dbus/interface.py +++ b/supervisor/dbus/interface.py @@ -29,12 +29,18 @@ class DBusInterface(ABC): name: str | None = None bus_name: str | None = None object_path: str | None = None + _shutdown: bool = False @property def is_connected(self) -> bool: """Return True, if they is connected to D-Bus.""" return self.dbus is not None + @property + def is_shutdown(self) -> bool: + """Return True, if the object has been shutdown.""" + return self._shutdown + async def connect(self, bus: MessageBus) -> None: """Connect to D-Bus.""" self.dbus = await DBus.connect(bus, self.bus_name, self.object_path) @@ -45,6 +51,14 @@ class DBusInterface(ABC): self.dbus.disconnect() self.dbus = None + def shutdown(self) -> None: + """Shutdown the object and disconnect from D-Bus. + + This method is irreversible. + """ + self._shutdown = True + self.disconnect() + class DBusInterfaceProxy(DBusInterface): """Handle D-Bus interface proxy.""" diff --git a/supervisor/dbus/manager.py b/supervisor/dbus/manager.py index 92456ddf8..706262acf 100644 --- a/supervisor/dbus/manager.py +++ b/supervisor/dbus/manager.py @@ -135,7 +135,7 @@ class DBusManager(CoreSysAttributes): return for dbus in self.all: - dbus.disconnect() + dbus.shutdown() self.bus.disconnect() _LOGGER.info("Closed conection to system D-Bus.") diff --git a/supervisor/dbus/network/__init__.py b/supervisor/dbus/network/__init__.py index 8427943d3..49606cb0b 100644 --- a/supervisor/dbus/network/__init__.py +++ b/supervisor/dbus/network/__init__.py @@ -1,5 +1,4 @@ """Network Manager implementation for DBUS.""" -import asyncio import logging from typing import Any @@ -215,18 +214,22 @@ class NetworkManager(DBusInterfaceProxy): for device in set(curr_devices.keys()) - set( self.properties[DBUS_ATTR_DEVICES] ): - asyncio.get_event_loop().run_in_executor( - None, curr_devices[device].disconnect - ) + curr_devices[device].shutdown() self._interfaces = interfaces + def shutdown(self) -> None: + """Shutdown the object and disconnect from D-Bus. + + This method is irreversible. + """ + self.dns.shutdown() + self.settings.shutdown() + super().shutdown() + def disconnect(self) -> None: """Disconnect from D-Bus.""" - self.dns.disconnect() - self.settings.disconnect() - for intr in self.interfaces.values(): - intr.disconnect() + intr.shutdown() super().disconnect() diff --git a/supervisor/dbus/network/connection.py b/supervisor/dbus/network/connection.py index a1f02c85f..b6e8baa00 100644 --- a/supervisor/dbus/network/connection.py +++ b/supervisor/dbus/network/connection.py @@ -1,6 +1,5 @@ """Connection object for Network Manager.""" -import asyncio from typing import Any from supervisor.dbus.network.setting import NetworkSetting @@ -82,7 +81,7 @@ class NetworkConnection(DBusInterfaceProxy): def settings(self, settings: NetworkSetting | None) -> None: """Set settings.""" if self._settings and self._settings is not settings: - asyncio.get_event_loop().run_in_executor(None, self._settings.disconnect) + self._settings.shutdown() self._settings = settings @@ -95,7 +94,7 @@ class NetworkConnection(DBusInterfaceProxy): def ipv4(self, ipv4: IpConfiguration | None) -> None: """Set ipv4 configuration.""" if self._ipv4 and self._ipv4 is not ipv4: - asyncio.get_event_loop().run_in_executor(None, self._ipv4.disconnect) + self._ipv4.shutdown() self._ipv4 = ipv4 @@ -108,7 +107,7 @@ class NetworkConnection(DBusInterfaceProxy): def ipv6(self, ipv6: IpConfiguration | None) -> None: """Set ipv6 configuration.""" if self._ipv6 and self._ipv6 is not ipv6: - asyncio.get_event_loop().run_in_executor(None, self._ipv6.disconnect) + self._ipv6.shutdown() self._ipv6 = ipv6 @@ -166,12 +165,15 @@ class NetworkConnection(DBusInterfaceProxy): else: self.settings = None - def disconnect(self) -> None: - """Disconnect from D-Bus.""" + def shutdown(self) -> None: + """Shutdown the object and disconnect from D-Bus. + + This method is irreversible. + """ if self.ipv4: - self.ipv4.disconnect() + self.ipv4.shutdown() if self.ipv6: - self.ipv6.disconnect() + self.ipv6.shutdown() if self.settings: - self.settings.disconnect() - super().disconnect() + self.settings.shutdown() + super().shutdown() diff --git a/supervisor/dbus/network/interface.py b/supervisor/dbus/network/interface.py index 81714bd2a..bce44f2d4 100644 --- a/supervisor/dbus/network/interface.py +++ b/supervisor/dbus/network/interface.py @@ -1,6 +1,5 @@ """NetworkInterface object for Network Manager.""" -import asyncio from typing import Any from dbus_fast.aio.message_bus import MessageBus @@ -77,7 +76,7 @@ class NetworkInterface(DBusInterfaceProxy): def connection(self, connection: NetworkConnection | None) -> None: """Set connection for interface.""" if self._connection and self._connection is not connection: - asyncio.get_event_loop().run_in_executor(None, self._connection.disconnect) + self._connection.shutdown() self._connection = connection @@ -95,7 +94,7 @@ class NetworkInterface(DBusInterfaceProxy): def wireless(self, wireless: NetworkWireless | None) -> None: """Set wireless for interface.""" if self._wireless and self._wireless is not wireless: - asyncio.get_event_loop().run_in_executor(None, self._wireless.disconnect) + self._wireless.shutdown() self._wireless = wireless @@ -139,10 +138,13 @@ class NetworkInterface(DBusInterfaceProxy): self.wireless = NetworkWireless(self.object_path) await self.wireless.connect(self.dbus.bus) - def disconnect(self) -> None: - """Disconnect from D-Bus.""" + def shutdown(self) -> None: + """Shutdown the object and disconnect from D-Bus. + + This method is irreversible. + """ if self.connection: - self.connection.disconnect() + self.connection.shutdown() if self.wireless: - self.wireless.disconnect() - super().disconnect() + self.wireless.shutdown() + super().shutdown() diff --git a/supervisor/dbus/network/setting/__init__.py b/supervisor/dbus/network/setting/__init__.py index c57fe03fa..e046bd777 100644 --- a/supervisor/dbus/network/setting/__init__.py +++ b/supervisor/dbus/network/setting/__init__.py @@ -167,13 +167,6 @@ class NetworkSetting(DBusInterface): self.dbus.Settings.Connection.on_updated(self.reload) - def disconnect(self) -> None: - """Disconnect from D-Bus.""" - if self.is_connected: - self.dbus.Settings.Connection.off_updated(self.reload) - - super().disconnect() - @dbus_connected async def reload(self): """Get current settings for connection.""" diff --git a/supervisor/dbus/network/wireless.py b/supervisor/dbus/network/wireless.py index 8b636d115..ac27123a5 100644 --- a/supervisor/dbus/network/wireless.py +++ b/supervisor/dbus/network/wireless.py @@ -41,7 +41,7 @@ class NetworkWireless(DBusInterfaceProxy): def active(self, active: NetworkWirelessAP | None) -> None: """Set active wireless AP.""" if self._active and self._active is not active: - asyncio.get_event_loop().run_in_executor(None, self._active.disconnect) + self._active.shutdown() self._active = active diff --git a/supervisor/dbus/utils.py b/supervisor/dbus/utils.py index bce8f4c40..f5cfdeaca 100644 --- a/supervisor/dbus/utils.py +++ b/supervisor/dbus/utils.py @@ -3,11 +3,17 @@ from ..exceptions import DBusNotConnectedError +async def noop(): + """Return a noop coroutine.""" + + def dbus_connected(method): """Wrap check if D-Bus is connected.""" def wrap_dbus(api, *args, **kwargs): """Check if D-Bus is connected before call a method.""" + if api.is_shutdown: + return noop() if api.dbus is None: raise DBusNotConnectedError() return method(api, *args, **kwargs) diff --git a/supervisor/utils/dbus.py b/supervisor/utils/dbus.py index 4a10ad1b1..be39b2175 100644 --- a/supervisor/utils/dbus.py +++ b/supervisor/utils/dbus.py @@ -193,9 +193,12 @@ class DBus: for intr, signals in self._signal_monitors.items(): for name, callbacks in signals.items(): for callback in callbacks: - getattr(self._proxies[intr], f"off_{name}")( - callback, unpack_variants=True - ) + try: + getattr(self._proxies[intr], f"off_{name}")( + callback, unpack_variants=True + ) + except Exception: # pylint: disable=broad-except + _LOGGER.exception("Can't remove signal listener") self._signal_monitors = {} diff --git a/tests/dbus/test_interface.py b/tests/dbus/test_interface.py index 6aa54a950..93ca88065 100644 --- a/tests/dbus/test_interface.py +++ b/tests/dbus/test_interface.py @@ -7,16 +7,16 @@ from unittest.mock import MagicMock from dbus_fast.aio.message_bus import MessageBus import pytest -from supervisor.dbus.interface import DBusInterface, DBusInterfaceProxy +from supervisor.dbus.interface import DBusInterfaceProxy from tests.common import fire_property_change_signal, fire_watched_signal @dataclass -class DBusInterfaceMock: +class DBusInterfaceProxyMock: """DBus Interface and signalling mocks.""" - obj: DBusInterface + obj: DBusInterfaceProxy on_device_added: MagicMock = MagicMock() off_device_added: MagicMock = MagicMock() @@ -24,7 +24,7 @@ class DBusInterfaceMock: @pytest.fixture(name="proxy") async def fixture_proxy( request: pytest.FixtureRequest, dbus_bus: MessageBus, dbus -) -> DBusInterfaceMock: +) -> DBusInterfaceProxyMock: """Get a proxy.""" proxy = DBusInterfaceProxy() proxy.bus_name = "org.freedesktop.NetworkManager" @@ -37,7 +37,7 @@ async def fixture_proxy( # pylint: disable=protected-access nm_proxy = proxy.dbus._proxies["org.freedesktop.NetworkManager"] - mock = DBusInterfaceMock(proxy) + mock = DBusInterfaceProxyMock(proxy) setattr(nm_proxy, "on_device_added", mock.on_device_added) setattr(nm_proxy, "off_device_added", mock.off_device_added) @@ -45,7 +45,7 @@ async def fixture_proxy( @pytest.mark.parametrize("proxy", [True], indirect=True) -async def test_dbus_proxy_connect(dbus_bus: MessageBus, proxy: DBusInterfaceMock): +async def test_dbus_proxy_connect(proxy: DBusInterfaceProxyMock): """Test dbus proxy connect.""" assert proxy.obj.is_connected assert proxy.obj.properties["Connectivity"] == 4 @@ -56,9 +56,7 @@ async def test_dbus_proxy_connect(dbus_bus: MessageBus, proxy: DBusInterfaceMock @pytest.mark.parametrize("proxy", [False], indirect=True) -async def test_dbus_proxy_connect_no_sync( - dbus_bus: MessageBus, proxy: DBusInterfaceMock -): +async def test_dbus_proxy_connect_no_sync(proxy: DBusInterfaceProxyMock): """Test dbus proxy connect with no properties sync.""" assert proxy.obj.is_connected assert proxy.obj.properties["Connectivity"] == 4 @@ -68,9 +66,7 @@ async def test_dbus_proxy_connect_no_sync( @pytest.mark.parametrize("proxy", [False], indirect=True) -async def test_signal_listener_disconnect( - dbus_bus: MessageBus, proxy: DBusInterfaceMock -): +async def test_signal_listener_disconnect(proxy: DBusInterfaceProxyMock): """Test disconnect/delete unattaches signal listeners.""" assert proxy.obj.is_connected device = None @@ -90,3 +86,23 @@ async def test_signal_listener_disconnect( proxy.obj.disconnect() proxy.off_device_added.assert_called_once_with(callback, unpack_variants=True) + + +@pytest.mark.parametrize("proxy", [False], indirect=True) +async def test_dbus_proxy_shutdown_pending_task(proxy: DBusInterfaceProxyMock): + """Test pending task does not raise DBusNotConnectedError after shutdown.""" + assert proxy.obj.is_connected + device = None + + async def callback(dev: str): + nonlocal device + await proxy.obj.update() + device = dev + + proxy.obj.dbus.on_device_added(callback) + fire_watched_signal( + proxy.obj, "org.freedesktop.NetworkManager.DeviceAdded", ["/test/obj/1"] + ) + proxy.obj.shutdown() + await asyncio.sleep(0) + assert device == "/test/obj/1"