From 0f4b79d24b11d52cdc8af900940d9c730835ed27 Mon Sep 17 00:00:00 2001 From: Jiri Hnidek Date: Wed, 1 Nov 2023 16:41:20 +0100 Subject: [PATCH] RHEL-15110: RegisterServer is stopped, when not needed * When some application starts RegisterServer using Start(), then it returns address. When this method is called multiple times and RegisterServer is still running, then the same address is returned. It means, when user starts multiple applications, then all of these applications should be able to use the same address to keep backward compatibility. The RegisterServer should be terminated only in the case, when the last application call Stop() and there is no running proccess using this RegisterServer. * This change does not allow to stop RegisterServer by some other user or application using Stop() method, when RegisterServer is still needed. * Non-root user can stop the RegisterServer only in the situation, when it is not needed by any application. * Implementation of Stop() method did not return anything explicitly, but the D-Bus method was designed to return boolean value. Thus, None object was interpreted as "False" value. It was fixed and it returns "True", when it was really stoped and it return "False", when it was not possible to stop it, because some application still uses RegisterServer. * It looks like that there was probably intention to use similar approach, but it has never been finished. * Modified some unit tests --- src/rhsmlib/client_info.py | 2 +- src/rhsmlib/dbus/dbus_utils.py | 7 +++++ src/rhsmlib/dbus/objects/register.py | 21 ++++++++++++-- src/rhsmlib/dbus/server.py | 43 +++++++++++++++++++++++++++- test/rhsmlib/dbus/test_register.py | 20 +++++++++++-- 5 files changed, 85 insertions(+), 8 deletions(-) diff --git a/src/rhsmlib/client_info.py b/src/rhsmlib/client_info.py index a7132266d3..aed83ffbd5 100644 --- a/src/rhsmlib/client_info.py +++ b/src/rhsmlib/client_info.py @@ -58,7 +58,7 @@ def get_cmd_line(sender, bus=None): if bus is None: bus = dbus.SystemBus() cmd_line = dbus_utils.command_of_sender(bus, sender) - if cmd_line is not None and type(cmd_line) == str: + if cmd_line is not None and type(cmd_line) is str: # Store only first argument of command line (no argument including username or password) cmd_line = cmd_line.split()[0] return cmd_line diff --git a/src/rhsmlib/dbus/dbus_utils.py b/src/rhsmlib/dbus/dbus_utils.py index 17c149114c..9968935d4a 100644 --- a/src/rhsmlib/dbus/dbus_utils.py +++ b/src/rhsmlib/dbus/dbus_utils.py @@ -48,6 +48,13 @@ def pid_of_sender(bus, sender): pid = int(dbus_iface.GetConnectionUnixProcessID(sender)) except ValueError: return None + # It seems that Python D-Bus implementation contains error. This should not happen, + # when we try to call GetConnectionUnixProcessID() with sender that does not exist + # anymore + except dbus.exceptions.DBusException as err: + log.debug(f"D-Bus raised exception: {err}") + return None + return pid diff --git a/src/rhsmlib/dbus/objects/register.py b/src/rhsmlib/dbus/objects/register.py index c31151ddf0..3b03419606 100644 --- a/src/rhsmlib/dbus/objects/register.py +++ b/src/rhsmlib/dbus/objects/register.py @@ -51,7 +51,13 @@ def start(self, sender: str) -> str: :return: Server address. """ with self.lock: + # When some other application already started domain socket listener, then + # write log message and return existing address if self.server is not None: + log.debug(f"Domain socket listener already running, started by: {self.server.sender}") + # Add sender to the list of senders using server + log.debug(f"Adding another sender {sender} to the set of senders") + self.server.add_sender(sender) return self.server.address log.debug("Trying to create new domain socket server.") @@ -66,7 +72,7 @@ def start(self, sender: str) -> str: ) return address - def stop(self) -> None: + def stop(self, sender: str) -> bool: """Stop the server running on the domain socket. :raises exceptions.Failed: No domain socket server is running. @@ -75,9 +81,18 @@ def stop(self) -> None: if self.server is None: raise exceptions.Failed("No domain socket server is running") + # Remove current sender and check if other senders are still running. + # If there is at least one sender using this server still running, then + # only return False + self.server.remove_sender(sender) + if self.server.are_other_senders_still_running() is True: + log.debug("Not stopping domain socket server, because some senders still uses it.") + return False + self.server.shutdown() self.server = None log.debug("Domain socket server stopped.") + return True class RegisterDBusObject(base_object.BaseObject): @@ -109,11 +124,11 @@ def Start(self, locale, sender=None): ) @util.dbus_handle_sender @util.dbus_handle_exceptions - def Stop(self, locale, sender=None): + def Stop(self, locale, sender=None) -> bool: locale = dbus_utils.dbus_to_python(locale, expected_type=str) Locale.set(locale) - self.impl.stop() + return self.impl.stop(sender) class OrgNotSpecifiedException(dbus.DBusException): diff --git a/src/rhsmlib/dbus/server.py b/src/rhsmlib/dbus/server.py index 5a9865cf8c..909a2dfaa1 100644 --- a/src/rhsmlib/dbus/server.py +++ b/src/rhsmlib/dbus/server.py @@ -27,6 +27,7 @@ from functools import partial from rhsmlib.services import config +from rhsmlib.dbus.dbus_utils import pid_of_sender from rhsm.config import get_config_parser from rhsmlib.file_monitor import create_filesystem_watcher, DirectoryWatch from rhsmlib.file_monitor import ( @@ -323,13 +324,53 @@ def __init__(self, object_classes=None, sender=None, cmd_line=None): self.object_classes = object_classes or [] self.objects = [] self._server = None - self.sender = sender + self.sender = sender # sender created the server + self._senders = set() # other senders using server + log.debug(f"Adding sender {sender} to the set of senders") + self._senders.add(sender) self.cmd_line = cmd_line self.lock = threading.Lock() with self.lock: self.connection_count = 0 + def add_sender(self, sender: str) -> None: + """ + Add sender to the list of senders + """ + self._senders.add(sender) + + def remove_sender(self, sender: str) -> bool: + """ + Try to remove sender from the set of sender + """ + try: + self._senders.remove(sender) + except KeyError: + log.debug(f"Sender {sender} wasn't removed from the set of senders (not member of the set)") + return False + else: + log.debug(f"Sender {sender} removed from the set of senders") + return True + + def are_other_senders_still_running(self) -> bool: + """ + Check if other users are still running. It sender in the set is not + running, then remove sender from the set, because sender could + crash, or it was terminated since it called Start() method. + """ + is_one_sender_running = False + not_running = set() + bus = dbus.SystemBus() + for sender in self._senders: + pid = pid_of_sender(bus, sender) + if pid is None: + not_running.add(sender) + else: + is_one_sender_running = True + self._senders = self._senders.difference(not_running) + return is_one_sender_running + def shutdown(self): for o in self.objects: o.remove_from_connection() diff --git a/test/rhsmlib/dbus/test_register.py b/test/rhsmlib/dbus/test_register.py index 671d9b15c3..e4934e8849 100644 --- a/test/rhsmlib/dbus/test_register.py +++ b/test/rhsmlib/dbus/test_register.py @@ -340,12 +340,26 @@ def setUp(self) -> None: get_cmd_line_mock = get_cmd_line_patch.start() get_cmd_line_mock.return_value = "unit-test sender" + pid_of_sender_patch = mock.patch( + "rhsmlib.dbus.server.pid_of_sender", + autospec=True, + ) + pid_of_sender_mock = pid_of_sender_patch.start() + pid_of_sender_mock.return_value = 123456 + + are_others_running_path = mock.patch( + "rhsmlib.dbus.server.DomainSocketServer.are_other_senders_still_running", + autospec=True, + ) + are_others_running_mock = are_others_running_path.start() + are_others_running_mock.return_value = False + dbus.mainloop.glib.DBusGMainLoop(set_as_default=True) def tearDown(self) -> None: """Make sure the domain server is stopped once the test ends.""" with contextlib.suppress(rhsmlib.dbus.exceptions.Failed): - self.impl.stop() + self.impl.stop("sender") super().tearDown() @@ -373,11 +387,11 @@ def test_Start__can_connect(self): def test_Stop(self): self.impl.start("sender") - self.impl.stop() + self.impl.stop("sender") def test_Stop__not_running(self): with self.assertRaises(rhsmlib.dbus.exceptions.Failed): - self.impl.stop() + self.impl.stop("sender") class DomainSocketRegisterDBusObjectTest(SubManDBusFixture):