bpo-45401: Change shouldRollover() methods to only rollover regular f…

[vsajip]

…iles.

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

https://bugs.python.org/issue45401

[serhiy-storchaka]

isfile() returns False if the file does not exist. Please add tests for delay=True.

[vsajip]

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?

[serhiy-storchaka]

I am worrying that returning False here prevents opening a file few lines below. Is it okay? Is it tested?

[vsajip]

Here's the logic common to the two handlers:

cpython/Lib/logging/handlers.py

Lines 65 to 77 in b108db6

	defemit(self, record):
	"""
	Emit a record.
	Output the record to the file, catering for rollover as described
	in doRollover().
	"""
	try:
	ifself.shouldRollover(record):
	self.doRollover()
	logging.FileHandler.emit(self, record)
	exceptException:
	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.

[vsajip]

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.

[serhiy-storchaka]

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.

[vsajip]

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]

Thanks for the review and approval, Serhiy!

[miss-islington]

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]

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]

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

[bedevere-bot]

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