Skip to content

gh-116263: Do not rollover empty files in RotatingFileHandler #122788

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions Lib/logging/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -196,9 +196,12 @@ def shouldRollover(self, record):
if self.stream is None: # delay was set...
self.stream = self._open()
if self.maxBytes > 0: # are we rolling over?
pos = self.stream.tell()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section will clash with the other PR for gh-116267, perhaps wait until that is finalised?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR is simpler, and it adds some tests for delay=True, so I think that it would be better to merge it first.

if not pos:
# gh-116263: Never rollover an empty file
return False
msg = "%s\n" % self.format(record)
self.stream.seek(0, 2) #due to non-posix-compliant Windows feature
if self.stream.tell() + len(msg) >= self.maxBytes:
if pos + len(msg) >= self.maxBytes:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The preceding line hardcodes the \n terminator - in the PR for gh-116267, I've changed it to avoid that. same comment as above re. clash with the PR for gh-116267.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but it will be not difficult to resolve conflict.

# See bpo-45401: Never rollover anything other than regular files
if os.path.exists(self.baseFilename) and not os.path.isfile(self.baseFilename):
return False
Expand Down
54 changes: 51 additions & 3 deletions Lib/test/test_logging.py
Original file line number Diff line number Diff line change
Expand Up @@ -6196,13 +6196,28 @@ def test_emit_after_closing_in_write_mode(self):
self.assertEqual(fp.read().strip(), '1')

class RotatingFileHandlerTest(BaseFileTest):
@unittest.skipIf(support.is_wasi, "WASI does not have /dev/null.")
def test_should_not_rollover(self):
# If maxbytes is zero rollover never occurs
# If file is empty rollover never occurs
rh = logging.handlers.RotatingFileHandler(
self.fn, encoding="utf-8", maxBytes=1)
self.assertFalse(rh.shouldRollover(None))
rh.close()

# If maxBytes is zero rollover never occurs
rh = logging.handlers.RotatingFileHandler(
self.fn, encoding="utf-8", maxBytes=0)
self.assertFalse(rh.shouldRollover(None))
rh.close()

with open(self.fn, 'wb') as f:
f.write(b'\n')
rh = logging.handlers.RotatingFileHandler(
self.fn, encoding="utf-8", maxBytes=0)
self.assertFalse(rh.shouldRollover(None))
rh.close()

@unittest.skipIf(support.is_wasi, "WASI does not have /dev/null.")
def test_should_not_rollover_non_file(self):
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not know what to do with this test. It is now passed not because the file is not a regular file, but because tell() returns 0. It always returns 0 for /dev/null on Linux.

So this test no longer tests what it was intended to test, and I do not know how to fix this. Is the check for regular file is even needed in shouldRollover()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'd need to check the behaviour on macOS and Windows too, though it seems like the zero should be returned on all platforms. This check can remain to test that we never rollover special files, no matter the internal reason. If we can guarantee that tell() returns 0 in all cases for all special files (not just /dev/null), perhaps the check for a regular file could be removed.

# bpo-45401 - test with special file
# We set maxBytes to 1 so that rollover would normally happen, except
# for the check for regular files
Expand All @@ -6212,18 +6227,47 @@ def test_should_not_rollover(self):
rh.close()

def test_should_rollover(self):
rh = logging.handlers.RotatingFileHandler(self.fn, encoding="utf-8", maxBytes=1)
with open(self.fn, 'wb') as f:
f.write(b'\n')
rh = logging.handlers.RotatingFileHandler(self.fn, encoding="utf-8", maxBytes=2)
self.assertTrue(rh.shouldRollover(self.next_rec()))
rh.close()

def test_file_created(self):
# checks that the file is created and assumes it was created
# by us
os.unlink(self.fn)
rh = logging.handlers.RotatingFileHandler(self.fn, encoding="utf-8")
rh.emit(self.next_rec())
self.assertLogFile(self.fn)
rh.close()

def test_max_bytes(self, delay=False):
kwargs = {'delay': delay} if delay else {}
os.unlink(self.fn)
rh = logging.handlers.RotatingFileHandler(
self.fn, encoding="utf-8", backupCount=2, maxBytes=100, **kwargs)
self.assertIs(os.path.exists(self.fn), not delay)
small = logging.makeLogRecord({'msg': 'a'})
large = logging.makeLogRecord({'msg': 'b'*100})
self.assertFalse(rh.shouldRollover(small))
self.assertFalse(rh.shouldRollover(large))
rh.emit(small)
self.assertLogFile(self.fn)
self.assertFalse(os.path.exists(self.fn + ".1"))
self.assertFalse(rh.shouldRollover(small))
self.assertTrue(rh.shouldRollover(large))
rh.emit(large)
self.assertTrue(os.path.exists(self.fn))
self.assertLogFile(self.fn + ".1")
self.assertFalse(os.path.exists(self.fn + ".2"))
self.assertTrue(rh.shouldRollover(small))
self.assertTrue(rh.shouldRollover(large))
rh.close()

def test_max_bytes_delay(self):
self.test_max_bytes(delay=True)

def test_rollover_filenames(self):
def namer(name):
return name + ".test"
Expand All @@ -6232,11 +6276,15 @@ def namer(name):
rh.namer = namer
rh.emit(self.next_rec())
self.assertLogFile(self.fn)
self.assertFalse(os.path.exists(namer(self.fn + ".1")))
rh.emit(self.next_rec())
self.assertLogFile(namer(self.fn + ".1"))
self.assertFalse(os.path.exists(namer(self.fn + ".2")))
rh.emit(self.next_rec())
self.assertLogFile(namer(self.fn + ".2"))
self.assertFalse(os.path.exists(namer(self.fn + ".3")))
rh.emit(self.next_rec())
self.assertFalse(os.path.exists(namer(self.fn + ".3")))
rh.close()

def test_namer_rotator_inheritance(self):
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
:class:`logging.handlers.RotatingFileHandler` no longer rolls over empty log
files.
Loading