-
-
Notifications
You must be signed in to change notification settings - Fork 31.4k
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
bpo-45401: Change shouldRollover() methods to only rollover regular f… #28822
Conversation
…iles. Also changed some historical return values from 1->True and 0->False.
Lib/logging/handlers.py
Outdated
@@ -187,14 +187,17 @@ def shouldRollover(self, record): | |||
Basically, see if the supplied record would cause the file to exceed | |||
the size limit we have. | |||
""" | |||
# See bpo-45401: Never rollover anything other than regular files | |||
if not os.path.isfile(self.baseFilename): |
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.
isfile()
returns False if the file does not exist. Please add tests for delay=True
.
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.
Fair point. But we're not checking for the file to be open, only if it's a regular file. So a test of
os.path.exists(X) and not os.path.isfile(X)
should be sufficient, because if it doesn't exist, it will be opened as a regular file either now or after a delay. If it exists, then the test will filter out devices, fifos etc. Have I missed anything else?
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 am worrying that returning False here prevents opening a file few lines below. Is it okay? Is it tested?
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.
Here's the logic common to the two handlers:
cpython/Lib/logging/handlers.py
Lines 65 to 77 in b108db6
def emit(self, record): | |
""" | |
Emit a record. | |
Output the record to the file, catering for rollover as described | |
in doRollover(). | |
""" | |
try: | |
if self.shouldRollover(record): | |
self.doRollover() | |
logging.FileHandler.emit(self, record) | |
except Exception: | |
self.handleError(record) |
We always write the logged message to the file, opening it if necessary, but with an optional rollover done by doRollover()
. What that does is:
- Close the existing file we're logging to (named by
self.baseFilename
). - Rename that file to a backup file, holding older log messages.
- If no delay is set, open a new file named by
self.baseFilename
, ready to receive the latest message - this is written via line 75 of the segment above. (If delay is set, the file will be opened in the code called at line 75).
Step 3 causes a problem if self.baseFilename
points to /dev/null
and the code is running as root, or if self.baseFilename
points to a FIFO, or some other non-regular file. The change I'm proposing would prevent doRollover()
running only if a file exists and is not a regular file (e.g. /dev/null
, or an existing FIFO). That means that the log continues to be written to exactly the same file as before - which seems appropriate.
For completeness, I should add a test on POSIX which uses os.mkfifo
to create a FIFO, and make the same checks as for os.devnull
. I will do this.
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.
For completeness, I should add a test on POSIX which uses os.mkfifo to create a FIFO,
Thinking about this, I don't think you can pass a FIFO name to a FileHandler
and expect it to work, since they block until the other side does something.
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 can run a thread which reads from a pipe.
On Windows you can use files \\.\pipe\...
. There are examples of using them in tests.
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 get that, but I'm saying that we don't need to go to that much trouble for this test, where the pipe functionality is ancillary. The not os.path.isfile(X)
will catch this case, so it should work when a user does this with an actual FIFO ... it seems unnecessary to spin up a thread here just to test this.
Thanks for the review and approval, Serhiy! |
Thanks @vsajip for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9. |
Thanks @vsajip for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10. |
GH-28866 is a backport of this pull request to the 3.9 branch. |
GH-28867 is a backport of this pull request to the 3.10 branch. |
pythonGH-28822) …iles. Also changed some historical return values from 1 -> True and 0 -> False. (cherry picked from commit 62a6677) Co-authored-by: Vinay Sajip <vinay_sajip@yahoo.co.uk>
pythonGH-28822) …iles. Also changed some historical return values from 1 -> True and 0 -> False. (cherry picked from commit 62a6677) Co-authored-by: Vinay Sajip <vinay_sajip@yahoo.co.uk>
…iles.
Also changed some historical return values from 1 -> True and 0 -> False.
https://bugs.python.org/issue45401