gh-116263: Do not rollover empty files in RotatingFileHandler #122788
Conversation
| rh.close() | ||
|
|
||
| @unittest.skipIf(support.is_wasi, "WASI does not have /dev/null.") | ||
| def test_should_not_rollover_non_file(self): |
There was a problem hiding this comment.
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()?
There was a problem hiding this comment.
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.
| @@ -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() | |||
There was a problem hiding this comment.
This section will clash with the other PR for gh-116267, perhaps wait until that is finalised?
There was a problem hiding this comment.
This PR is simpler, and it adds some tests for delay=True, so I think that it would be better to merge it first.
| 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: |
There was a problem hiding this comment.
Yes, but it will be not difficult to resolve conflict.
| rh.close() | ||
|
|
||
| @unittest.skipIf(support.is_wasi, "WASI does not have /dev/null.") | ||
| def test_should_not_rollover_non_file(self): |
There was a problem hiding this comment.
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.
Misc/NEWS.d/next/Library/2024-08-07-17-41-16.gh-issue-116263.EcXir0.rst
Outdated
Show resolved
Hide resolved
| @@ -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() | |||
There was a problem hiding this comment.
This PR is simpler, and it adds some tests for delay=True, so I think that it would be better to merge it first.
| 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: |
There was a problem hiding this comment.
Yes, but it will be not difficult to resolve conflict.
Misc/NEWS.d/next/Library/2024-08-07-17-41-16.gh-issue-116263.EcXir0.rst
Outdated
Show resolved
Hide resolved
|
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13. |
…ythonGH-122788) (cherry picked from commit 6094c6f) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
GH-122814 is a backport of this pull request to the 3.13 branch. |
…ythonGH-122788) (cherry picked from commit 6094c6f) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
|
GH-122815 is a backport of this pull request to the 3.12 branch. |
Uh oh!
There was an error while loading. Please reload this page.