bpo-33978: close resources before deletion of logging handlers by tirkarthi · Pull Request #8008 · python/cpython

tirkarthi · 2018-06-29T12:36:19Z

Close the file handlers before deleting the logging handlers so that there is no resource warning. Since the handlers are a weakreference I tried to close them by calling self.close on __del__ but not all handlers have the lock attribute which resulted in AttributeError. shutdown handles the AttributeError and other scenarios but it will be a much cleaner if we close the handlers during weakref deletion instead of calling shudown which might miss some places.

Additionally shutdown is also registered at exit which should clean up the current handlers I hope but doesn't handle the ones that were deleted.

Attached tests and a NEWS entry. Kindly correct me if I am wrong on the approach since this is my first non-easy PR and I would like some help on this fix hoping the fix will not be much complex.

Thanks

https://bugs.python.org/issue33978

tirkarthi · 2018-06-29T13:03:01Z

Windows builds for fileConfig fails with unicodeescape error and the one for dictConfig fails with permission error to open the file though tests pass on Linux and MacOS. I don't have a windows machine to test this and any pointers will be helpful in fixing the tests.

vsajip · 2018-06-30T13:09:48Z

    logging._acquireLock()
    try:
        logging._handlers.clear()
+


The 3 lines

logging._handlers.clear() logging.shutdown(logging._handlerList[:]) del logging._handlerList[:]

are now in two places - at line 76 and line 531. I suggest adding a module-level function _clearExistingHandlers() which does these operations, along with two calls to them from those locations.

Also, logging.shutdown() takes _handlerList as a default argument, so it is not necessary to pass the argument to it.

vsajip · 2018-06-30T13:11:40Z

@@ -0,0 +1,2 @@
+Close resources before deleting logging handlers. Patch by Karthikeyan


Change to "Closed existing logging handlers before reconfiguration via fileConfig and dictConfig".

vsajip · 2018-06-30T13:21:41Z

+    keys=root
+
+    [handlers]
+    keys=file_handler


Could just name as file rather than file_handler. The section below then becomes [handler_file] rather than [handler_file_handler].

vsajip · 2018-06-30T13:22:21Z

+    def test_config8_ok(self):
+        with self.check_no_resource_warning():
+            with tempfile.NamedTemporaryFile() as f:
+                self.apply_config(self.config8.format(tempfile=f.name))


You could do the formatting just once to a local string, and then use that.

vsajip · 2018-06-30T13:25:52Z


+    def test_config8_ok(self):
+        with self.check_no_resource_warning():
+            with tempfile.NamedTemporaryFile() as f:


Use the approach used elsewhere in this file with fd, fn = tempfile.mkstemp(".log", "test_logging-X-") followed by closing the fd. This allows tracing of where the temp file came from, in the odd case where it isn't deleted for some reason. It should also ensure that the file is writable, or fail at this point rather than later.

bedevere-bot · 2018-07-01T21:31:16Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

tirkarthi · 2018-07-02T06:34:43Z

I have made the requested changes; please review again

bedevere-bot · 2018-07-02T06:34:45Z

Thanks for making the requested changes!

@vsajip: please review the changes made to this pull request.

tirkarthi · 2018-07-02T06:49:39Z

The windows build fails due to the below :

With respect fileConfig test formatting the string results in path having \Users and hence a unicodeescape error is produced. I have tried raw string suggested in SO answer but as noted in the comments it seems not to work for ''' quoted strings and I have to manually escape \ myself as per the comment.
With respect to dictConfig I can see two errors. One in VSTS there is permissionDenied error which I hope is due to permission error but there is a comment in f315df3 that This exception is thrown when a directory with the chosen name already exists on windows. . In AppVeyor I cannot see the error and hence the file is created successfully but it seems that dicConfig uses the file handler and hence os.unlink(fn) fails since windows doesn't allow deleting files already used by the process. But trying to close the handler before calling os.unlink kind of removes the point of testing the resource leak.

os.remove

On Windows, attempting to remove a file that is in use causes an exception to be raised; on Unix, the directory entry is removed but the storage allocated to the file is not made available until the original file is no longer in use.

I don't have a windows machine to test this and if mkstemp failure is due to the name being already present then I don't know why I could see it in other tests like test_tempfile.py. Perhaps there is a better way to test this I am not aware of?

Thanks

tirkarthi · 2018-07-02T08:47:17Z

@vsajip I think I have fixed the issues with respect to windows.

I have added code to replace '\' with '\\' as I came across another gotcha where using raw string makes it look like it's escaping the backslash but actually only escapes while printing and the backslashes are not escaped in the string.
The case where deleting a file while it's used by another process in windows I have added a cleanup method like other tests in the file where the handler is closed and then the actual file is deleted safely using os.remove. I have verified that manually doing it doesn't affect the test cases for warnings by removing the _cleanExistingHandlers call that causes the warnings to happen in the tests.

On an additional note I think this bug is also present in Python 2.7 and I hope a backport is feasible. Ref :

cpython/Lib/logging/config.py

Line 83 in 9b84cc8

del logging._handlerList[:]

Thanks

vsajip · 2018-07-02T08:52:07Z

The case where deleting a file while it's used by another process in windows I have added a cleanup method like other tests in the file where the handler is closed and then the actual file is deleted safely using os.remove.

Removing the file shouldn't be needed, just closing it should be enough. If you can delete the file, it means it can't be held open in another process. I assume you're deleting it solely because it's a temp file that's no longer needed.

tirkarthi · 2018-07-02T08:57:14Z

Sorry, to be clear I am not deleting the file in logging handler I am talking about the test case related scenario in mkstemp where the file is not deleted automatically and the user has to delete the file on his own. Since the file is used only by the single test process then I think the file has to be deleted in the test case at the end in the cleanup method. Correct me if I am wrong or misunderstood your reply.

Thanks

vsajip · 2018-07-02T08:58:08Z

Thanks for your work on this.

tirkarthi · 2018-07-02T08:59:18Z

Sure, thanks much for your review comments and guidance. Does this need to be backported to 3.7, 3.6 and 2.7?

miss-islington · 2018-07-02T09:02:03Z

Thanks @tirkarthi for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-07-02T09:02:33Z

GH-8044 is a backport of this pull request to the 3.7 branch.

…pythonGH-8008) (cherry picked from commit 087570a) Co-authored-by: Xtreak <tirkarthi@users.noreply.github.com>

miss-islington · 2018-07-02T09:03:11Z

Thanks @tirkarthi for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6.
🐍🍒⛏🤖

miss-islington · 2018-07-02T09:03:11Z

Thanks @tirkarthi for the PR, and @vsajip for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

miss-islington · 2018-07-02T09:03:14Z

Sorry, @tirkarthi and @vsajip, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 087570af6d5d39b51bdd5e660a53903960e58678 3.6

…ation. (pythonGH-8008). (cherry picked from commit 087570a) Co-authored-by: Xtreak <tirkarthi@users.noreply.github.com>

bedevere-bot · 2018-07-02T09:07:50Z

GH-8045 is a backport of this pull request to the 3.6 branch.

…GH-8008) (GH-8044) (cherry picked from commit 087570a) Co-authored-by: Xtreak <tirkarthi@users.noreply.github.com>

…ation. (GH-8008). (GH-8045) (cherry picked from commit 087570a) Co-authored-by: Xtreak <tirkarthi@users.noreply.github.com>

flyte · 2021-02-11T09:47:10Z

Close the file handlers before deleting the logging handlers so that there is no resource warning.

Don't you only want to do this if disable_existing_loggers == True?

This change is causing the socket on configured SysLogHandlers to be closed, throwing an error when they're used, for example:

import logging.config
import socket
from logging.handlers import SysLogHandler

log = logging.getLogger("logtest")
syslog_handler = SysLogHandler(("127.0.0.1", 12345), socktype=socket.SOCK_DGRAM)
log.addHandler(syslog_handler)

log.warning("Works")

logging.config.dictConfig(
    {
        "version": 1,
        "disable_existing_loggers": False,
    }
)

log.warning("Breaks")

Causes:

--- Logging error ---
Traceback (most recent call last):
  File "/usr/lib/python3.8/logging/handlers.py", line 940, in emit
    self.socket.sendto(msg, self.address)
OSError: [Errno 9] Bad file descriptor
Call stack:
  File "/home/flyte/workspaces/python/test.py", line 18, in <module>
    log.warning("Breaks")

It's a valid case where multiple calls to dictConfig are legitimately done by separate parts of a system (gunicorn and Django for example), so one call (by Django) should not affect the existing resources configured by the other (gunicorn).

tirkarthi added 2 commits June 29, 2018 12:20

Close resources before deleting logging handlers

7e6a027

Add NEWS entry

d54a3f6

the-knights-who-say-ni added the CLA signed label Jun 29, 2018

bedevere-bot added the awaiting review label Jun 29, 2018

vsajip added skip news and removed skip news labels Jun 30, 2018

vsajip requested changes Jul 1, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Jul 1, 2018

tirkarthi added 4 commits July 2, 2018 05:18

Change NEWS entry

698abe6

Use _clearExistingHandlers to avoid duplicate code

9fd2066

Use mkstemp instead of NamedTemporaryFile

d986a36

Use raw string to avoid unicodeescape error in Windows

4be6fbe

bedevere-bot added awaiting change review and removed awaiting changes labels Jul 2, 2018

Replace backslash and handle open fd in windows

b79f8a2

vsajip approved these changes Jul 2, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Jul 2, 2018

vsajip merged commit 087570a into python:master Jul 2, 2018

bedevere-bot removed the awaiting merge label Jul 2, 2018

vsajip added the needs backport to 3.7 label Jul 2, 2018

bedevere-bot removed the needs backport to 3.7 label Jul 2, 2018

vsajip added needs backport to 3.6 labels Jul 2, 2018

miss-islington assigned vsajip Jul 2, 2018

tirkarthi mentioned this pull request Jul 2, 2018

[3.6] bpo-33978: Close existing handlers before logging (re-)configuration. (GH-8008) #8045

Merged

bedevere-bot removed the needs backport to 3.6 label Jul 2, 2018

vsajip pushed a commit that referenced this pull request Jul 2, 2018

bpo-33978: Close existing handlers before logging (re-)configuration. (…

6f49afc

…GH-8008) (GH-8044) (cherry picked from commit 087570a) Co-authored-by: Xtreak <tirkarthi@users.noreply.github.com>

vsajip pushed a commit that referenced this pull request Jul 2, 2018

[3.6] bpo-33978: Close existing handlers before logging (re-)configur…

b6c1989

…ation. (GH-8008). (GH-8045) (cherry picked from commit 087570a) Co-authored-by: Xtreak <tirkarthi@users.noreply.github.com>

Mariatta removed the needs backport to 3.7 label Jul 30, 2018

Agent-Hellboy mentioned this pull request Dec 6, 2024

logging.config.dictConfig() with "disable_existing_loggers=False" closes existing handlers #127567

Closed

		@@ -0,0 +1,2 @@
		Close resources before deleting logging handlers. Patch by Karthikeyan

Uh oh!

Conversation

tirkarthi commented Jun 29, 2018 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tirkarthi commented Jun 29, 2018

Uh oh!

vsajip Jun 30, 2018

Choose a reason for hiding this comment

Uh oh!

vsajip Jun 30, 2018

Choose a reason for hiding this comment

Uh oh!

vsajip Jun 30, 2018

Choose a reason for hiding this comment

Uh oh!

vsajip Jun 30, 2018

Choose a reason for hiding this comment

Uh oh!

vsajip Jun 30, 2018

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Jul 1, 2018

Uh oh!

tirkarthi commented Jul 2, 2018

Uh oh!

bedevere-bot commented Jul 2, 2018

Uh oh!

tirkarthi commented Jul 2, 2018

Uh oh!

tirkarthi commented Jul 2, 2018

Uh oh!

vsajip commented Jul 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tirkarthi commented Jul 2, 2018

Uh oh!

vsajip commented Jul 2, 2018

Uh oh!

tirkarthi commented Jul 2, 2018

Uh oh!

miss-islington commented Jul 2, 2018

Uh oh!

bedevere-bot commented Jul 2, 2018

Uh oh!

miss-islington commented Jul 2, 2018

Uh oh!

miss-islington commented Jul 2, 2018

Uh oh!

miss-islington commented Jul 2, 2018

Uh oh!

bedevere-bot commented Jul 2, 2018

Uh oh!

flyte commented Feb 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tirkarthi commented Jun 29, 2018 •

edited by bedevere-bot

Loading

vsajip commented Jul 2, 2018 •

edited

Loading

flyte commented Feb 11, 2021 •

edited

Loading