From 566c07c78955c2940fac7cc71051b6114c6e8676 Mon Sep 17 00:00:00 2001 From: tarneo Date: Wed, 21 Feb 2024 21:15:10 +0100 Subject: Allow reloading config on SIGHUP Before this commit qutebrowser would just exit when receiving a SIGHUP. Now it will reload the config using the bare config_source function, just like the :config-source command does. Closes #8108. --- qutebrowser/misc/crashsignal.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/qutebrowser/misc/crashsignal.py b/qutebrowser/misc/crashsignal.py index c69dcbe29..79dbe6bcc 100644 --- a/qutebrowser/misc/crashsignal.py +++ b/qutebrowser/misc/crashsignal.py @@ -335,6 +335,8 @@ class SignalHandler(QObject): signal.SIGINT, self.interrupt) self._orig_handlers[signal.SIGTERM] = signal.signal( signal.SIGTERM, self.interrupt) + self._orig_handlers[signal.SIGHUP] = signal.signal( + signal.SIGHUP, self.reload_config) if utils.is_posix and hasattr(signal, 'set_wakeup_fd'): # pylint: disable=import-error,no-member,useless-suppression @@ -430,6 +432,15 @@ class SignalHandler(QObject): print("WHY ARE YOU DOING THIS TO ME? :(") sys.exit(128 + signum) + def reload_config(self, _signum, _frame): + """Reload the config.""" + log.signals.info("SIGHUP received, reloading config.") + config_commands = objreg.get('config-commands', from_command=True) + try: + config_commands.config_source() + except cmdutils.CommandError as e: + log.signals.error("Error while reloading config:", exc_info=e) + def init(q_app: QApplication, args: argparse.Namespace, -- cgit v1.2.3-54-g00ecf From d5926756aec477bc2c56a620b3d84ea494245e34 Mon Sep 17 00:00:00 2001 From: tarneo Date: Thu, 22 Feb 2024 11:21:20 +0100 Subject: Replicate config_source in SIGHUP handler --- qutebrowser/misc/crashsignal.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/qutebrowser/misc/crashsignal.py b/qutebrowser/misc/crashsignal.py index 79dbe6bcc..be69ace6e 100644 --- a/qutebrowser/misc/crashsignal.py +++ b/qutebrowser/misc/crashsignal.py @@ -21,6 +21,8 @@ from qutebrowser.qt.core import (pyqtSlot, qInstallMessageHandler, QObject, QSocketNotifier, QTimer, QUrl) from qutebrowser.qt.widgets import QApplication +from qutebrowser.config import configfiles, configexc + from qutebrowser.api import cmdutils from qutebrowser.misc import earlyinit, crashdialog, ipc, objects from qutebrowser.utils import usertypes, standarddir, log, objreg, debug, utils @@ -435,11 +437,11 @@ class SignalHandler(QObject): def reload_config(self, _signum, _frame): """Reload the config.""" log.signals.info("SIGHUP received, reloading config.") - config_commands = objreg.get('config-commands', from_command=True) + filename = standarddir.config_py() try: - config_commands.config_source() - except cmdutils.CommandError as e: - log.signals.error("Error while reloading config:", exc_info=e) + configfiles.read_config_py(filename) + except configexc.ConfigFileErrors as e: + raise cmdutils.CommandError(e) def init(q_app: QApplication, -- cgit v1.2.3-54-g00ecf From aafb44cbaaabd8a1e99af27108573dbf592724fd Mon Sep 17 00:00:00 2001 From: tarneo Date: Thu, 22 Feb 2024 11:26:22 +0100 Subject: Run message.error() on SIGHUP config errors --- qutebrowser/misc/crashsignal.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/qutebrowser/misc/crashsignal.py b/qutebrowser/misc/crashsignal.py index be69ace6e..277ae53bf 100644 --- a/qutebrowser/misc/crashsignal.py +++ b/qutebrowser/misc/crashsignal.py @@ -25,7 +25,7 @@ from qutebrowser.config import configfiles, configexc from qutebrowser.api import cmdutils from qutebrowser.misc import earlyinit, crashdialog, ipc, objects -from qutebrowser.utils import usertypes, standarddir, log, objreg, debug, utils +from qutebrowser.utils import usertypes, standarddir, log, objreg, debug, utils, message from qutebrowser.qt import sip if TYPE_CHECKING: from qutebrowser.misc import quitter @@ -441,7 +441,7 @@ class SignalHandler(QObject): try: configfiles.read_config_py(filename) except configexc.ConfigFileErrors as e: - raise cmdutils.CommandError(e) + message.error(str(e)) def init(q_app: QApplication, -- cgit v1.2.3-54-g00ecf From f6c0e4ebc81b1ef1afb1fb71d103a42f39d62779 Mon Sep 17 00:00:00 2001 From: toofar Date: Sat, 24 Feb 2024 21:50:38 +1300 Subject: Add some basic tests for SignalHandler It doesn't look like this class has any unit tests currently. And since we are adding a new signal handler to it I took the opportunity to add a few tests to it to establish a bit of a framework so that next time we touch it it will be easier to add more. I didn't go for full coverage here. It's an improvement over what was there previously and it should make merging more palatable. I don't think handling SIGHUP is a very risky feature. I chose to use mocker.patch over monkeypatch.setattr because I think it provides a more friendly and powerful API due to being more tightly integrated with magic mocks. And with `mocker.patch.object` you can even use actual object names instead of strings, the same as monkeypatch allows. One thing I'm less confident with here is mocking all multiple things in a couple of fixtures. Writing one fixture for each little thing I mock doesn't feel appealing to me right now, but for mocks that tests need to access there isn't really any idiomatic way to let the tests access them. I previously just had the one fixture but pulled out the read config one for that purpose. It's a pattern I'll have to think on a bit I think. Probably having to have developers thing about the balance of boilerplate vs accessibility is cognitive load that we want to avoid. Hmm. Anyway, here are the options I was looking at to let test code access mocks that where all shoved away into the one fixture: 1. group the tests into a class and put the mocks in class variables: could work fine but I don't think it's very idiomatic for pytest? 2. return some kind of meta object from the fixture that has the object under test as one attribute and the mocks as other ones: hmm, returning multiple values from a method always seemed a bit smelly to me 3. make one fixture for each of the mocks, have the main fixture depend on them all, tests that need to modify them can depend on them too: writing all those fixtures seems tedious and I don't think it would be a clear win in terms of readability. *sigh*, I suppose I should pull the ones I'm modifying out at least so other people don't copy my lazyness in the future 4. have the test code access the mocks from the real code, eg `configfiles.sys.exit.side_effect = ...`: this works fine but I feel like it's relying on an implementation detail and probably shouldn't be encouraged? Not sure In case anyone is struggling with git blame and wondering what the QSocketNotifier stuff is about, this commit should explain it: https://github.com/qutebrowser/qutebrowser/commit/210ce8ca7cb16e1d0ce859baa64ce56a5e1894cf --- qutebrowser/misc/crashsignal.py | 17 +++--- tests/unit/misc/test_crashsignal.py | 102 ++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 8 deletions(-) create mode 100644 tests/unit/misc/test_crashsignal.py diff --git a/qutebrowser/misc/crashsignal.py b/qutebrowser/misc/crashsignal.py index 277ae53bf..daffa6341 100644 --- a/qutebrowser/misc/crashsignal.py +++ b/qutebrowser/misc/crashsignal.py @@ -21,9 +21,8 @@ from qutebrowser.qt.core import (pyqtSlot, qInstallMessageHandler, QObject, QSocketNotifier, QTimer, QUrl) from qutebrowser.qt.widgets import QApplication -from qutebrowser.config import configfiles, configexc - from qutebrowser.api import cmdutils +from qutebrowser.config import configfiles, configexc from qutebrowser.misc import earlyinit, crashdialog, ipc, objects from qutebrowser.utils import usertypes, standarddir, log, objreg, debug, utils, message from qutebrowser.qt import sip @@ -324,6 +323,12 @@ class SignalHandler(QObject): self._activated = False self._orig_wakeup_fd: Optional[int] = None + self._handlers = { + signal.SIGINT: self.interrupt, + signal.SIGTERM: self.interrupt, + signal.SIGHUP: self.reload_config, + } + def activate(self): """Set up signal handlers. @@ -333,12 +338,8 @@ class SignalHandler(QObject): On Unix, it uses a QSocketNotifier with os.set_wakeup_fd to get notified. """ - self._orig_handlers[signal.SIGINT] = signal.signal( - signal.SIGINT, self.interrupt) - self._orig_handlers[signal.SIGTERM] = signal.signal( - signal.SIGTERM, self.interrupt) - self._orig_handlers[signal.SIGHUP] = signal.signal( - signal.SIGHUP, self.reload_config) + for sig, handler in self._handlers.items(): + self._orig_handlers[sig] = signal.signal(sig, handler) if utils.is_posix and hasattr(signal, 'set_wakeup_fd'): # pylint: disable=import-error,no-member,useless-suppression diff --git a/tests/unit/misc/test_crashsignal.py b/tests/unit/misc/test_crashsignal.py new file mode 100644 index 000000000..e4570420e --- /dev/null +++ b/tests/unit/misc/test_crashsignal.py @@ -0,0 +1,102 @@ +# SPDX-FileCopyrightText: Florian Bruhin (The Compiler) +# +# SPDX-License-Identifier: GPL-3.0-or-later + +"""Tests for qutebrowser.misc.crashsignal.""" + +import signal + +import pytest + +from qutebrowser.config import configexc +from qutebrowser.qt.widgets import QApplication +from qutebrowser.misc import crashsignal, quitter + + +@pytest.fixture +def read_config_mock(mocker): + # covers reload_config + mocker.patch.object( + crashsignal.standarddir, + "config_py", + return_value="config.py-unittest", + ) + return mocker.patch.object( + crashsignal.configfiles, + "read_config_py", + autospec=True, + ) + + +@pytest.fixture +def signal_handler(qtbot, mocker, read_config_mock): + """Signal handler instance with all external methods mocked out.""" + # covers init + mocker.patch.object(crashsignal.sys, "exit", autospec=True) + signal_handler = crashsignal.SignalHandler( + app=mocker.Mock(spec=QApplication), + quitter=mocker.Mock(spec=quitter.Quitter), + ) + + return signal_handler + + +def test_handlers_registered(signal_handler): + signal_handler.activate() + + for sig, handler in signal_handler._handlers.items(): + registered = signal.signal(sig, signal.SIG_DFL) + assert registered == handler + + +def test_handlers_deregistered(signal_handler): + known_handler = lambda *_args: None + for sig in signal_handler._handlers: + signal.signal(sig, known_handler) + + signal_handler.activate() + signal_handler.deactivate() + + for sig in signal_handler._handlers: + registered = signal.signal(sig, signal.SIG_DFL) + assert registered == known_handler + + +def test_interrupt_repeatedly(signal_handler): + signal_handler.activate() + test_signal = signal.SIGINT + + expected_handlers = [ + signal_handler.interrupt, + signal_handler.interrupt_forcefully, + signal_handler.interrupt_really_forcefully, + ] + + # Call the SIGINT handler multiple times and make sure it calls the + # expected sequence of functions. + for expected in expected_handlers: + registered = signal.signal(test_signal, signal.SIG_DFL) + assert registered == expected + expected(test_signal, None) + + +def test_reload_config_call_on_hup(signal_handler, read_config_mock): + signal_handler._handlers[signal.SIGHUP](None, None) + + read_config_mock.assert_called_once_with("config.py-unittest") + + +def test_reload_config_displays_errors(signal_handler, read_config_mock, mocker): + read_config_mock.side_effect = configexc.ConfigFileErrors( + "config.py", + [ + configexc.ConfigErrorDesc("no config.py", ValueError("asdf")) + ] + ) + message_mock = mocker.patch.object(crashsignal.message, "error") + + signal_handler._handlers[signal.SIGHUP](None, None) + + message_mock.assert_called_once_with( + "Errors occurred while reading config.py:\n no config.py: asdf" + ) -- cgit v1.2.3-54-g00ecf From 6050017809a2980934e626a6da5418b07838478a Mon Sep 17 00:00:00 2001 From: toofar Date: Sat, 24 Feb 2024 22:32:46 +1300 Subject: update changelog for SIGHUP handling --- doc/changelog.asciidoc | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/doc/changelog.asciidoc b/doc/changelog.asciidoc index 91b02b0da..89e33679e 100644 --- a/doc/changelog.asciidoc +++ b/doc/changelog.asciidoc @@ -19,6 +19,12 @@ breaking changes (such as renamed commands) can happen in minor releases. v3.2.0 (unreleased) ------------------- +Added +~~~~~ + +- When qutebrowser receives a SIGHUP it will now reload any config.py file + in use (same as the `:config-source` command does). (#8108) + Changed ~~~~~~~ -- cgit v1.2.3-54-g00ecf From 145bfe4de0802e7eb21ef902e8e53544e996d3a4 Mon Sep 17 00:00:00 2001 From: toofar Date: Sun, 25 Feb 2024 15:38:10 +1300 Subject: windows doesn't support SIGHUP It gives an AttributeError for both signal.SIGHUP and signal.Signals.SIGHUP. The Signals Enum is set up so you can use the strings to key into it though, so that's nice. I was tempted to use a walrus operator here but I think that's python 310 and I don't remember what our minimum supported version is. --- qutebrowser/misc/crashsignal.py | 7 ++++++- tests/unit/misc/test_crashsignal.py | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/qutebrowser/misc/crashsignal.py b/qutebrowser/misc/crashsignal.py index daffa6341..05e5806df 100644 --- a/qutebrowser/misc/crashsignal.py +++ b/qutebrowser/misc/crashsignal.py @@ -326,8 +326,13 @@ class SignalHandler(QObject): self._handlers = { signal.SIGINT: self.interrupt, signal.SIGTERM: self.interrupt, - signal.SIGHUP: self.reload_config, } + platform_dependant_handlers = { + "SIGHUP": self.reload_config, + } + for sig_str, handler in platform_dependant_handlers.items(): + if hasattr(signal.Signals, sig_str): + self._handlers[signal.Signals[sig_str]] = handler def activate(self): """Set up signal handlers. diff --git a/tests/unit/misc/test_crashsignal.py b/tests/unit/misc/test_crashsignal.py index e4570420e..7019118e5 100644 --- a/tests/unit/misc/test_crashsignal.py +++ b/tests/unit/misc/test_crashsignal.py @@ -80,12 +80,14 @@ def test_interrupt_repeatedly(signal_handler): expected(test_signal, None) +@pytest.mark.posix def test_reload_config_call_on_hup(signal_handler, read_config_mock): signal_handler._handlers[signal.SIGHUP](None, None) read_config_mock.assert_called_once_with("config.py-unittest") +@pytest.mark.posix def test_reload_config_displays_errors(signal_handler, read_config_mock, mocker): read_config_mock.side_effect = configexc.ConfigFileErrors( "config.py", -- cgit v1.2.3-54-g00ecf