Fix hang when removing unbound connections

proton/vpn/backend/networkmanager/killswitch/default/nmclient.py

@@ -28,6 +28,7 @@
 from packaging.version import Version
 
 import gi
+
 gi.require_version("NM", "1.0")
 from gi.repository import NM, GLib, Gio, GObject  # pylint: disable=C0413 # noqa: E402
 
@@ -130,7 +131,7 @@ def __init__(self):
         self.initialize_nm_client_singleton()
 
     def add_connection_async(
-        self, connection: NM.Connection, save_to_disk: bool = False
+            self, connection: NM.Connection, save_to_disk: bool = False
     ) -> Future:
         """
         Adds a new connection asynchronously.
@@ -228,11 +229,21 @@ def _on_connection_removed(connection, result, _user_data):
                         f"Error removing KS connection: {connection=}, {result=}"
                     ).with_traceback(exc.__traceback__)
                 )
+            #  If the connection to remove has no live device bound to it will not emit a
+            #  "device-removed" signal. Removal is completed as the connection is deleted, thus we
+            #  set the result here to avoid hanging.
+            else:
+                interface_name: str = connection.get_interface_name()
+                if self._has_named_device(interface_name) or future_interface_removed.done():
+                    return
+                future_interface_removed.set_result(None)
 
         def _on_interface_removed(_nm_client, device):
             logger.debug(
                 f"{device.get_iface()} was removed."
             )
+            if future_interface_removed.done():
+                return
             if device.get_iface() == connection.get_interface_name():
                 future_interface_removed.set_result(None)
 
@@ -254,12 +265,28 @@ def _remove_connection_async():
 
         return future_interface_removed
 
+    def _has_named_device(self, interface_name: str) -> bool:
+        """
+        Returns whether NetworkManager currently has a device for the given
+        interface name.
+
+        This must be called from the GLib main loop thread (e.g. from within a
+        NetworkManager async callback).
+        """
+        for device in self._nm_client.get_devices():
+            if device.get_iface() == interface_name:
+                break
+        else:
+            return False
+        return True
+
     def get_active_connection(self, conn_id: str) -> Optional[NM.ActiveConnection]:
         """
         Returns the specified active connection, if existing.
         :param conn_id: ID of the active connection.
         :return: the active connection if it was found. Otherwise, None.
         """
+
         def _get_active_connection():
             active_connections = self._nm_client.get_active_connections()
 

proton/vpn/backend/networkmanager/killswitch/wireguard/nmclient.py

@@ -28,6 +28,7 @@
 from packaging.version import Version
 
 import gi
+
 gi.require_version("NM", "1.0")
 from gi.repository import NM, GLib, Gio, GObject  # pylint: disable=C0413 # noqa: E402
 
@@ -218,9 +219,9 @@ def get_physical_devices(self) -> List[NM.Device]:
         """Returns all the active ethernet/wifi devices."""
         return [
             device for device in self._nm_client.get_devices() if (
-                device.get_device_type() in (NM.DeviceType.ETHERNET, NM.DeviceType.WIFI)
-                and device.get_state() is NM.DeviceState.ACTIVATED
-                and device.get_active_connection()  # Maybe this is redundant.
+                    device.get_device_type() in (NM.DeviceType.ETHERNET, NM.DeviceType.WIFI)
+                    and device.get_state() is NM.DeviceState.ACTIVATED
+                    and device.get_active_connection()  # Maybe this is redundant.
             )
         ]
 
@@ -431,12 +432,23 @@ def _on_connection_removed(connection, result, _user_data):
                         f"Error removing KS connection: {exc}"
                     ).with_traceback(exc.__traceback__)
                 )
+            #  If the connection to remove has no live device bound to it will not emit a
+            #  "device-removed" signal. Removal is completed as the connection is deleted, thus we
+            #  set the result here to avoid hanging.
+            else:
+                interface_name: str = connection.get_interface_name()
+                if self._has_named_device(interface_name) or future_interface_removed.done():
+                    return
+                future_interface_removed.set_result(None)
 
         def _on_interface_removed(_nm_client, device):
             logger.debug(
                 f"{device.get_iface()} was removed."
             )
-            if device.get_iface() == connection.get_interface_name():
+            if (
+                    device.get_iface() == connection.get_interface_name()
+                    and not future_interface_removed.done()
+            ):
                 future_interface_removed.set_result(None)
 
         def _remove_connection_async():
@@ -457,12 +469,28 @@ def _remove_connection_async():
 
         return future_interface_removed
 
+    def _has_named_device(self, interface_name: str) -> bool:
+        """
+        Returns whether NetworkManager currently has a device for the given
+        interface name.
+
+        This must be called from the GLib main loop thread (e.g. from within a
+        NetworkManager async callback).
+        """
+        for device in self._nm_client.get_devices():
+            if device.get_iface() == interface_name:
+                break
+        else:
+            return False
+        return True
+
     def get_active_connection(self, conn_id: str) -> Optional[NM.ActiveConnection]:
         """
         Returns the specified active connection, if existing.
         :param conn_id: ID of the active connection.
         :return: the active connection if it was found. Otherwise, None.
         """
+
         def _get_active_connection():
             active_connections = self._nm_client.get_active_connections()
 

tests/networkmanager/killswitch/default/test_nmclient.py

@@ -0,0 +1,106 @@
+"""
+Copyright (c) 2023 Proton AG
+
+This file is part of Proton VPN.
+
+Proton VPN is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+Proton VPN is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with ProtonVPN.  If not, see <https://www.gnu.org/licenses/>.
+"""
+from concurrent.futures import Future
+from unittest.mock import Mock, patch
+
+import pytest
+
+from proton.vpn.backend.networkmanager.killswitch.default.nmclient import NMClient
+
+INTERFACE_NAME = "pvpnksintrf0"
+
+
+def _run_synchronously(function, *args, **kwargs):
+    """Drop-in replacement for NMClient._run_on_glib_loop_thread that runs the
+    given function synchronously instead of scheduling it on the GLib loop."""
+    future = Future()
+    future.set_running_or_notify_cancel()
+    try:
+        future.set_result(function(*args, **kwargs))
+    except BaseException as exc:  # pylint: disable=broad-except
+        future.set_exception(exc)
+    return future
+
+
+@pytest.fixture
+def nm_client():
+    with patch.object(NMClient, "initialize_nm_client_singleton"), \
+            patch.object(NMClient, "_run_on_glib_loop_thread", side_effect=_run_synchronously), \
+            patch("proton.vpn.backend.networkmanager.killswitch.default.nmclient.GObject"):
+        client = NMClient()
+        client._nm_client = Mock()
+        yield client
+
+
+def _build_connection():
+    connection = Mock()
+    connection.get_interface_name.return_value = INTERFACE_NAME
+    connection.delete_finish.return_value = True
+
+    # Simulate NetworkManager finishing the deletion synchronously by invoking
+    # the callback as soon as delete_async is called.
+    def delete_async(cancellable, callback, user_data):  # noqa: ARG001
+        callback(connection, Mock(), user_data)
+
+    connection.delete_async.side_effect = delete_async
+    return connection
+
+
+def test_remove_connection_resolves_immediately_when_there_is_no_interface(nm_client):
+    """When the connection being removed has no active interface, no
+    device-removed signal will ever be emitted, so the removal must be
+    considered complete as soon as the connection is deleted."""
+    nm_client._nm_client.get_devices.return_value = []  # no devices at all
+    connection = _build_connection()
+
+    future = nm_client.remove_connection_async(connection)
+
+    assert future.done()
+    assert future.result() is None
+
+
+def test_remove_connection_waits_for_device_removed_when_interface_is_active(nm_client):
+    """When the connection being removed has an active interface, the removal
+    is only complete once the corresponding device has actually been removed."""
+    device = Mock()
+    device.get_iface.return_value = INTERFACE_NAME
+    nm_client._nm_client.get_devices.return_value = [device]
+
+    # Capture the handler connected to the "device-removed" signal.
+    handlers = {}
+
+    def connect(signal_name, handler):
+        handlers[signal_name] = handler
+        return 1  # handler id
+
+    nm_client._nm_client.connect.side_effect = connect
+
+    connection = _build_connection()
+
+    future = nm_client.remove_connection_async(connection)
+
+    # The connection was deleted, but the device is still present, so the
+    # future must not be resolved until the device is actually removed.
+    assert not future.done()
+
+    # Simulate NetworkManager emitting the device-removed signal.
+    handlers["device-removed"](nm_client._nm_client, device)
+
+    assert future.done()
+    assert future.result() is None

tests/networkmanager/killswitch/wireguard/test_nmclient.py

@@ -0,0 +1,106 @@
+"""
+Copyright (c) 2023 Proton AG
+
+This file is part of Proton VPN.
+
+Proton VPN is free software: you can redistribute it and/or modify
+it under the terms of the GNU General Public License as published by
+the Free Software Foundation, either version 3 of the License, or
+(at your option) any later version.
+
+Proton VPN is distributed in the hope that it will be useful,
+but WITHOUT ANY WARRANTY; without even the implied warranty of
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+GNU General Public License for more details.
+
+You should have received a copy of the GNU General Public License
+along with ProtonVPN.  If not, see <https://www.gnu.org/licenses/>.
+"""
+from concurrent.futures import Future
+from unittest.mock import Mock, patch
+
+import pytest
+
+from proton.vpn.backend.networkmanager.killswitch.wireguard.nmclient import NMClient
+
+INTERFACE_NAME = "pvpnksintrf0"
+
+
+def _run_synchronously(function, *args, **kwargs):
+    """Drop-in replacement for NMClient._run_on_glib_loop_thread that runs the
+    given function synchronously instead of scheduling it on the GLib loop."""
+    future = Future()
+    future.set_running_or_notify_cancel()
+    try:
+        future.set_result(function(*args, **kwargs))
+    except BaseException as exc:  # pylint: disable=broad-except
+        future.set_exception(exc)
+    return future
+
+
+@pytest.fixture
+def nm_client():
+    with patch.object(NMClient, "initialize_nm_client_singleton"), \
+            patch.object(NMClient, "_run_on_glib_loop_thread", side_effect=_run_synchronously), \
+            patch("proton.vpn.backend.networkmanager.killswitch.wireguard.nmclient.GObject"):
+        client = NMClient()
+        client._nm_client = Mock()
+        yield client
+
+
+def _build_connection():
+    connection = Mock()
+    connection.get_interface_name.return_value = INTERFACE_NAME
+    connection.delete_finish.return_value = True
+
+    # Simulate NetworkManager finishing the deletion synchronously by invoking
+    # the callback as soon as delete_async is called.
+    def delete_async(cancellable, callback, user_data):  # noqa: ARG001
+        callback(connection, Mock(), user_data)
+
+    connection.delete_async.side_effect = delete_async
+    return connection
+
+
+def test_remove_connection_resolves_immediately_when_there_is_no_interface(nm_client):
+    """When the connection being removed has no active interface, no
+    device-removed signal will ever be emitted, so the removal must be
+    considered complete as soon as the connection is deleted."""
+    nm_client._nm_client.get_devices.return_value = []  # no devices at all
+    connection = _build_connection()
+
+    future = nm_client.remove_connection_async(connection)
+
+    assert future.done()
+    assert future.result() is None
+
+
+def test_remove_connection_waits_for_device_removed_when_interface_is_active(nm_client):
+    """When the connection being removed has an active interface, the removal
+    is only complete once the corresponding device has actually been removed."""
+    device = Mock()
+    device.get_iface.return_value = INTERFACE_NAME
+    nm_client._nm_client.get_devices.return_value = [device]
+
+    # Capture the handler connected to the "device-removed" signal.
+    handlers = {}
+
+    def connect(signal_name, handler):
+        handlers[signal_name] = handler
+        return 1  # handler id
+
+    nm_client._nm_client.connect.side_effect = connect
+
+    connection = _build_connection()
+
+    future = nm_client.remove_connection_async(connection)
+
+    # The connection was deleted, but the device is still present, so the
+    # future must not be resolved until the device is actually removed.
+    assert not future.done()
+
+    # Simulate NetworkManager emitting the device-removed signal.
+    handlers["device-removed"](nm_client._nm_client, device)
+
+    assert future.done()
+    assert future.result() is None