Skip to content
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

Merged
merged 2 commits into from Oct 10, 2021

Conversation

vsajip
Copy link
Member

@vsajip vsajip commented Oct 8, 2021

…iles.

Also changed some historical return values from 1 -> True and 0 -> False.

https://bugs.python.org/issue45401

…iles.

Also changed some historical return values from 1->True and 0->False.
@@ -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):
Copy link
Member

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.

Copy link
Member Author

@vsajip vsajip Oct 8, 2021

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?

Copy link
Member

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?

Copy link
Member Author

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:

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:

  1. Close the existing file we're logging to (named by self.baseFilename).
  2. Rename that file to a backup file, holding older log messages.
  3. 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.

Copy link
Member Author

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.

Copy link
Member

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.

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 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.

@vsajip
Copy link
Member Author

vsajip commented Oct 10, 2021

Thanks for the review and approval, Serhiy!

@serhiy-storchaka serhiy-storchaka added needs backport to 3.9 only security fixes needs backport to 3.10 only security fixes labels Oct 11, 2021
@miss-islington
Copy link
Contributor

Thanks @vsajip for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@miss-islington
Copy link
Contributor

Thanks @vsajip for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

@bedevere-bot
Copy link

GH-28866 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label Oct 11, 2021
@bedevere-bot
Copy link

GH-28867 is a backport of this pull request to the 3.10 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Oct 11, 2021
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 11, 2021
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>
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Oct 11, 2021
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>
vstinner pushed a commit that referenced this pull request Oct 11, 2021
GH-28822) (#28866)

…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>

Co-authored-by: Vinay Sajip <vinay_sajip@yahoo.co.uk>
vstinner pushed a commit that referenced this pull request Oct 11, 2021
GH-28822) (#28867)

…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>

Co-authored-by: Vinay Sajip <vinay_sajip@yahoo.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants