-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
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 our terms of service and privacy 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?
Changes from 5 commits
410909f
a9103d5
7b724ff
ae805a2
7351db4
9a85157
53fb2eb
9d8343a
28f5169
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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[:]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the Don't we need all these actions here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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? |
||
del logging._handlerList[:] | ||
|
||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Avoid shutting down handlers during reconfiguration | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should clarify that this is related to the |
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, not before as it's happening now?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
cpython/Lib/logging/config.py
Line 907 in 2041a95
here as well to counter it , I will do that for fileconfig as well