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

@@ -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()

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."""

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.")

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()

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()

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()

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."""

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
 

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)

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 = {}
 

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"