From 410909f589b414fb53c26517d4619bf07422751b Mon Sep 17 00:00:00 2001 From: Prince Roshan Date: Fri, 6 Dec 2024 18:56:31 +0530 Subject: [PATCH 1/7] gh-127567: Avoid shuting down handlers during reconfiguration --- Lib/logging/config.py | 15 +++++++++++++-- Lib/test/test_logging.py | 26 ++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/Lib/logging/config.py b/Lib/logging/config.py index 6a6a7f726f7e0c..cf5cfdbc568099 100644 --- a/Lib/logging/config.py +++ b/Lib/logging/config.py @@ -285,9 +285,20 @@ def _install_loggers(cp, handlers, disable_existing): def _clearExistingHandlers(): - """Clear and close existing handlers""" + """Clear and close handlers that are no longer in use.""" + active_handlers = { + handler + for logger in logging.Logger.manager.loggerDict.values() + if isinstance(logger, logging.Logger) + for handler in logger.handlers + } + + for handler_ref in list(logging._handlers.values()): + handler = handler_ref() if callable(handler_ref) else handler_ref + if handler and handler not in active_handlers: + handler.close() + logging._handlers.clear() - logging.shutdown(logging._handlerList[:]) del logging._handlerList[:] diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index e72f222e1c7eeb..ff86a417d9b8cd 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -3554,6 +3554,32 @@ def test_config15_ok(self): handler = logging.root.handlers[0] self.addCleanup(closeFileHandler, handler, fn) + def test_clear_existing_handlers_preserves_active_handlers(self): + """Test that active handlers are preserved and unused handlers are closed.""" + + with self.check_no_resource_warning(): + fn = make_temp_file(".log", "test_logging-clear-") + config = { + "version": 1, + "handlers": { + "file": { + "class": "logging.FileHandler", + "filename": fn, + "encoding": "utf-8", + } + }, + "root": { + "handlers": ["file"] + } + } + + self.apply_config(config) + self.apply_config(config) + + handler = logging.root.handlers[0] + self.assertFalse(handler._closed, "Handler should not be closed") + self.addCleanup(closeFileHandler, handler, fn) + def test_config16_ok(self): self.apply_config(self.config16) h = logging._handlers['hand1'] From 7b724ff238b27bf6d0f137ea767831edb6527c4c Mon Sep 17 00:00:00 2001 From: Prince Roshan Date: Fri, 6 Dec 2024 22:06:12 +0530 Subject: [PATCH 2/7] Fix test --- Lib/test/test_logging.py | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index ff86a417d9b8cd..4c949c3cf4b71b 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -3554,31 +3554,26 @@ def test_config15_ok(self): handler = logging.root.handlers[0] self.addCleanup(closeFileHandler, handler, fn) - def test_clear_existing_handlers_preserves_active_handlers(self): - """Test that active handlers are preserved and unused handlers are closed.""" + def test_disable_existing_loggers(self): + fn = make_temp_file(".log", "test_logging-disable-existing-loggers-") + file_handler = logging.FileHandler(fn, mode="w") + file_handler.setFormatter(logging.Formatter("%(message)s")) + root_logger = logging.getLogger() + root_logger.addHandler(file_handler) - with self.check_no_resource_warning(): - fn = make_temp_file(".log", "test_logging-clear-") - config = { - "version": 1, - "handlers": { - "file": { - "class": "logging.FileHandler", - "filename": fn, - "encoding": "utf-8", - } - }, - "root": { - "handlers": ["file"] - } - } + config = {"version": 1, "disable_existing_loggers": False} - self.apply_config(config) - self.apply_config(config) + # we have disable_existing_loggers=False, + # so, all handlers should continue working + self.apply_config(config) - handler = logging.root.handlers[0] - self.assertFalse(handler._closed, "Handler should not be closed") - self.addCleanup(closeFileHandler, handler, fn) + msg = "test message" + logging.warning(msg) + file_handler.close() + with open(fn, encoding='utf-8') as f: + data = f.read().strip() + os.remove(fn) + self.assertEqual(data, msg) def test_config16_ok(self): self.apply_config(self.config16) From 7351db45febee049aed7b8fd3a6156193c5b477d Mon Sep 17 00:00:00 2001 From: "blurb-it[bot]" <43283697+blurb-it[bot]@users.noreply.github.com> Date: Fri, 6 Dec 2024 16:40:34 +0000 Subject: [PATCH 3/7] =?UTF-8?q?=F0=9F=93=9C=F0=9F=A4=96=20Added=20by=20blu?= =?UTF-8?q?rb=5Fit.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../next/Library/2024-12-06-16-40-27.gh-issue-127567.uuPayH.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Library/2024-12-06-16-40-27.gh-issue-127567.uuPayH.rst diff --git a/Misc/NEWS.d/next/Library/2024-12-06-16-40-27.gh-issue-127567.uuPayH.rst b/Misc/NEWS.d/next/Library/2024-12-06-16-40-27.gh-issue-127567.uuPayH.rst new file mode 100644 index 00000000000000..2ee63a5ee08820 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-12-06-16-40-27.gh-issue-127567.uuPayH.rst @@ -0,0 +1 @@ +Avoid shutting down handlers during reconfiguration From 9a85157ce5d3de7efe9c6994cd3aa3c7a231eac6 Mon Sep 17 00:00:00 2001 From: Prince Roshan Date: Sun, 8 Dec 2024 03:52:25 +0530 Subject: [PATCH 4/7] Add handlers_to_clear param to _clearExistingHandlers --- Lib/logging/config.py | 49 +++++++++++++++++++++++++++++-------------- 1 file changed, 33 insertions(+), 16 deletions(-) diff --git a/Lib/logging/config.py b/Lib/logging/config.py index cf5cfdbc568099..78e3821fc06e11 100644 --- a/Lib/logging/config.py +++ b/Lib/logging/config.py @@ -84,7 +84,13 @@ def fileConfig(fname, defaults=None, disable_existing_loggers=True, encoding=Non # critical section with logging._lock: - _clearExistingHandlers() + # Handle removing specific handlers if specified + remove_handlers = cp.defaults().get("remove_handlers", "") + remove_handlers = [h.strip() for h in remove_handlers.split(",") if h.strip()] + + # Clear only specified handlers + if disable_existing_loggers: + _clearExistingHandlers(remove_handlers) # Handlers add themselves to logging._handlers handlers = _install_handlers(cp, formatters) @@ -283,23 +289,31 @@ def _install_loggers(cp, handlers, disable_existing): # logger.disabled = 1 _handle_existing_loggers(existing, child_loggers, disable_existing) +def _clearExistingHandlers(handlers_to_clear=None): + """ + Remove and shutdown only the specified handlers. + """ + if not handlers_to_clear: + return + handlers_to_remove = [] -def _clearExistingHandlers(): - """Clear and close handlers that are no longer in use.""" - active_handlers = { - handler - for logger in logging.Logger.manager.loggerDict.values() - if isinstance(logger, logging.Logger) - for handler in logger.handlers - } + # Handle root logger handlers + root = logging.root + for handler in root.handlers[:]: + if handler.name in handlers_to_clear: + root.removeHandler(handler) + handlers_to_remove.append(handler) - for handler_ref in list(logging._handlers.values()): - handler = handler_ref() if callable(handler_ref) else handler_ref - if handler and handler not in active_handlers: - handler.close() + # Handle handlers from other loggers + for logger_name, logger in root.manager.loggerDict.items(): + if isinstance(logger, logging.Logger): + for handler in logger.handlers[:]: + if handler.name in handlers_to_clear: + logger.removeHandler(handler) + handlers_to_remove.append(handler) - logging._handlers.clear() - del logging._handlerList[:] + # Shutdown the removed handlers + logging.shutdown(handlers_to_remove) IDENTIFIER = re.compile('^[a-z_][a-z0-9_]*$', re.I) @@ -550,6 +564,8 @@ def configure(self): if config['version'] != 1: raise ValueError("Unsupported version: %s" % config['version']) incremental = config.pop('incremental', False) + remove_handlers = config.pop('remove_handlers', None) + EMPTY_DICT = {} with logging._lock: if incremental: @@ -585,7 +601,8 @@ def configure(self): else: disable_existing = config.pop('disable_existing_loggers', True) - _clearExistingHandlers() + if disable_existing: + _clearExistingHandlers(remove_handlers) # Do formatters first - they don't refer to anything else formatters = config.get('formatters', EMPTY_DICT) From 53fb2eb17aceffb8977a9a7a83b3f3be948aea49 Mon Sep 17 00:00:00 2001 From: Prince Roshan Date: Sun, 8 Dec 2024 03:55:53 +0530 Subject: [PATCH 5/7] Remove test --- Lib/test/test_logging.py | 20 -------------------- 1 file changed, 20 deletions(-) diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index 4c949c3cf4b71b..f1e0a83a6d4e8a 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -3554,26 +3554,6 @@ def test_config15_ok(self): handler = logging.root.handlers[0] self.addCleanup(closeFileHandler, handler, fn) - def test_disable_existing_loggers(self): - fn = make_temp_file(".log", "test_logging-disable-existing-loggers-") - file_handler = logging.FileHandler(fn, mode="w") - file_handler.setFormatter(logging.Formatter("%(message)s")) - root_logger = logging.getLogger() - root_logger.addHandler(file_handler) - - config = {"version": 1, "disable_existing_loggers": False} - - # we have disable_existing_loggers=False, - # so, all handlers should continue working - self.apply_config(config) - - msg = "test message" - logging.warning(msg) - file_handler.close() - with open(fn, encoding='utf-8') as f: - data = f.read().strip() - os.remove(fn) - self.assertEqual(data, msg) def test_config16_ok(self): self.apply_config(self.config16) From 9d8343a872ecafa9fa5faf741b15a1ebb616c558 Mon Sep 17 00:00:00 2001 From: Prince Roshan Date: Mon, 9 Dec 2024 14:57:31 +0530 Subject: [PATCH 6/7] Add missing disable_existing_handler check in configure_logger --- Lib/logging/config.py | 45 ++++++++++++++++++++++++++-------------- Lib/test/test_logging.py | 1 - 2 files changed, 29 insertions(+), 17 deletions(-) diff --git a/Lib/logging/config.py b/Lib/logging/config.py index 78e3821fc06e11..53fbfb6f342d5e 100644 --- a/Lib/logging/config.py +++ b/Lib/logging/config.py @@ -84,12 +84,10 @@ def fileConfig(fname, defaults=None, disable_existing_loggers=True, encoding=Non # critical section with logging._lock: - # Handle removing specific handlers if specified - remove_handlers = cp.defaults().get("remove_handlers", "") - remove_handlers = [h.strip() for h in remove_handlers.split(",") if h.strip()] - - # Clear only specified handlers if disable_existing_loggers: + remove_handlers = cp.defaults().get("remove_handlers", "") + remove_handlers = [h.strip() for h in remove_handlers.split(",") if h.strip()] + _clearExistingHandlers(remove_handlers) # Handlers add themselves to logging._handlers @@ -297,14 +295,25 @@ def _clearExistingHandlers(handlers_to_clear=None): return handlers_to_remove = [] - # Handle root logger handlers +def _clearExistingHandlers(handlers_to_clear=None): + """ + Remove and shutdown only the specified handlers. + """ + if not handlers_to_clear: + return + + handlers_to_remove = [] + root = logging.root + + # Remove from root logger for handler in root.handlers[:]: if handler.name in handlers_to_clear: root.removeHandler(handler) handlers_to_remove.append(handler) + handler.clear() - # Handle handlers from other loggers + # Remove from other loggers for logger_name, logger in root.manager.loggerDict.items(): if isinstance(logger, logging.Logger): for handler in logger.handlers[:]: @@ -312,9 +321,11 @@ def _clearExistingHandlers(handlers_to_clear=None): logger.removeHandler(handler) handlers_to_remove.append(handler) - # Shutdown the removed handlers logging.shutdown(handlers_to_remove) + del handlers_to_clear + + IDENTIFIER = re.compile('^[a-z_][a-z0-9_]*$', re.I) @@ -602,7 +613,8 @@ def configure(self): disable_existing = config.pop('disable_existing_loggers', True) if disable_existing: - _clearExistingHandlers(remove_handlers) + handlers_to_close = config.pop('remove_handlers',[]) + _clearExistingHandlers(handlers_to_close) # Do formatters first - they don't refer to anything else formatters = config.get('formatters', EMPTY_DICT) @@ -683,7 +695,7 @@ def configure(self): i += 1 existing.remove(name) try: - self.configure_logger(name, loggers[name]) + self.configure_logger(name, loggers[name], disable_existing_handler=disable_existing) except Exception as e: raise ValueError('Unable to configure logger ' '%r' % name) from e @@ -924,7 +936,7 @@ def add_handlers(self, logger, handlers): except Exception as e: raise ValueError('Unable to add handler %r' % h) from e - def common_logger_config(self, logger, config, incremental=False): + def common_logger_config(self, logger, config, incremental=False, disable_existing_handler=True): """ Perform configuration which is common to root and non-root loggers. """ @@ -932,9 +944,10 @@ def common_logger_config(self, logger, config, incremental=False): if level is not None: logger.setLevel(logging._checkLevel(level)) if not incremental: - #Remove any existing handlers - for h in logger.handlers[:]: - logger.removeHandler(h) + if disable_existing_handler: + #Remove any existing handlers if disable_existing_handler is True + for h in logger.handlers[:]: + logger.removeHandler(h) handlers = config.get('handlers', None) if handlers: self.add_handlers(logger, handlers) @@ -942,10 +955,10 @@ def common_logger_config(self, logger, config, incremental=False): if filters: self.add_filters(logger, filters) - def configure_logger(self, name, config, incremental=False): + def configure_logger(self, name, config, incremental=False, disable_existing_handler=True): """Configure a non-root logger from a dictionary.""" logger = logging.getLogger(name) - self.common_logger_config(logger, config, incremental) + self.common_logger_config(logger, config, incremental, disable_existing_handler) logger.disabled = False propagate = config.get('propagate', None) if propagate is not None: diff --git a/Lib/test/test_logging.py b/Lib/test/test_logging.py index f1e0a83a6d4e8a..e72f222e1c7eeb 100644 --- a/Lib/test/test_logging.py +++ b/Lib/test/test_logging.py @@ -3554,7 +3554,6 @@ def test_config15_ok(self): handler = logging.root.handlers[0] self.addCleanup(closeFileHandler, handler, fn) - def test_config16_ok(self): self.apply_config(self.config16) h = logging._handlers['hand1'] From 28f51699dc009ac3c97d9ceddfda15c016daae9f Mon Sep 17 00:00:00 2001 From: Prince Roshan Date: Mon, 9 Dec 2024 15:18:07 +0530 Subject: [PATCH 7/7] Clean config.py --- Lib/logging/config.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/Lib/logging/config.py b/Lib/logging/config.py index 53fbfb6f342d5e..11a0a0db1170c9 100644 --- a/Lib/logging/config.py +++ b/Lib/logging/config.py @@ -287,13 +287,6 @@ def _install_loggers(cp, handlers, disable_existing): # logger.disabled = 1 _handle_existing_loggers(existing, child_loggers, disable_existing) -def _clearExistingHandlers(handlers_to_clear=None): - """ - Remove and shutdown only the specified handlers. - """ - if not handlers_to_clear: - return - handlers_to_remove = [] def _clearExistingHandlers(handlers_to_clear=None): """