Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
gh-127567: Avoid forceful shutting down of handlers during reconfiguration#127690
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
Lib/test/test_logging.py Outdated
| @@ -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): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@Agent-Hellboy I don't understand how this test covers the problem described in the original issue... The problem in the issue is with handlers of loggers that are not supposed to be modified by new configuration, not with handlers defined in the configuration...
I would propose the following test:
deftest_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 workingself.apply_config(config)msg="test message"logging.warning(msg)file_handler.close()withopen(fn,encoding='utf-8')asf:data=f.read().strip()os.remove(fn)self.assertEqual(data,msg)
Agent-HellboyDec 6, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
okay, I was testingassert fileHandler in root_logger.handlers only, I have used yours test now
| @@ -0,0 +1 @@ | |||
| Avoid shutting down handlers during reconfiguration | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You should clarify that this is related to thelogging module.
Lib/logging/config.py Outdated
| """Clear and close handlers that are no longer in use.""" | ||
| active_handlers = { | ||
| handler | ||
| for logger in logging.Logger.manager.loggerDict.values() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The reason why now the test is passing is that the root logger is not included inmanager.loggerDict(). If you change the following line in the test
root_logger=logging.getLogger()
to
root_logger=logging.getLogger("any-logger")
the test will fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
sorry, I simply copied your test without giving a thought
Lib/logging/config.py Outdated
| @@ -285,9 +285,20 @@ def _install_loggers(cp, handlers, disable_existing): | |||
| def _clearExistingHandlers(): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't we call this_clearExistingHandlers()after we applied all the modifications from config, notbefore as it's happening now?
Agent-HellboyDec 9, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I have added a check
Line 907 in2041a95
| #Remove any existing handlers |
here as well to counter it , I will do that for fileconfig as well
Lib/logging/config.py Outdated
| @@ -285,9 +285,20 @@ def _install_loggers(cp, handlers, disable_existing): | |||
| def _clearExistingHandlers(): | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we rename the method to better reflect its behaviour? To something like_clearNotUsedHandlers.
| logging._handlers.clear() | ||
| logging.shutdown(logging._handlerList[:]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
thelogging.shutdown() method callshandler.acquire(),handler.flush() andhandler.release(). Also, it iterate all these handlers in a specific order (not sure why).
Don't we need all these actions here?
Agent-HellboyDec 7, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Also, it iterate all these handlers in a specific order (not sure why)
didn't get your question, it is to protect the handler in a concurrent environment. A mutex lock , acquire operation release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
My question is why your implementation in this PR doesn't used them?
aparshin commentedDec 9, 2024
@Agent-Hellboy I see the following issues in this PR:
|
Uh oh!
There was an error while loading.Please reload this page.