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.

Sign up for GitHub

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

Fixed an issue where the tail would fail in case of file rename and replace as in log rotation scenario. #22

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed an issue where the tail would fail in case of file rename and replace as in log rotation scenario. #22

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 20 additions & 0 deletions tailer/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import os
import re
import sys
import time
Expand Down Expand Up @@ -160,7 +161,17 @@ def follow(self, delay=1.0):
"""
trailing = True

initial_inode = os.stat(self.file.name).st_ino
Copy link
Owner

Choose a reason for hiding this comment

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

tailer is meant to work on streams as well. So an io.StringIO should work too. io.StringIO does not have a .name parameter so this would fail.

file_path = self.file.name

while 1:
Copy link
Owner

Choose a reason for hiding this comment

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

Use while True:. There's no need to optimize to using an int here. I see while 1 is used in other places. I'd consider that a bad practice now.

try:
if os.stat(self.file.name).st_ino != initial_inode:
self.file = open(file_path)
initial_inode = os.stat(self.file.name).st_ino
except FileNotFoundError:
continue
Copy link
Owner

Choose a reason for hiding this comment

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

This will get stuck in a tight infinite loop if the file does not exist. We need a non-blocking wait. The easiest way to do this is to introduce a time.sleep(delay) if the file does not exist.

See comments on https://github.com/six8/pytailer/pull/14/files#r1167325214

We want to mimic what GNU tail is doing.

With GNU tail, the -f option will follow the file by inode. So if the file is renamed, it will continue to follow the file at the new name. We want to continue that behavior.

GNU tail adds a -F option, which is an alias for --follow=name --retry

Note: In --follow=name, name is a constant value. The options are name or descriptor with descriptor being the default.

See the GNU tail docs for more info https://man7.org/linux/man-pages/man1/tail.1.html

The following tests needed to be added. Start with the tests, then the implementation will be more obvious:

  1. If using -f and the file is renamed, it continues to follow the file at the new name and ignores a file at the old name.
  2. If using -F (or --follow=name --retry) and the file is renamed, ignore the new name file and follow the old name file.

This means adding a new --retry and -F option and making it so --follow has 3 states:

  1. --follow
  2. --follow=name
  3. --follow=descriptor

You can see how GNU tail is implemented at https://github.com/coreutils/coreutils/blob/master/src/tail.c. It uses inotify, which we can't use because inotify is not part of the Python standard library, and the intention is to keep tailer pure-python and only depend on the standard library.


where = self.file.tell()
line = self.file.readline()
if line:
Expand Down Expand Up @@ -234,9 +245,18 @@ def follow(file, delay=1.0):
>>> f.flush()
>>> next(generator)
'Line 2'
>>> os.rename("test_follow.txt", "test_follow_1.txt")
Copy link
Owner

Choose a reason for hiding this comment

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

This is a separate test case. The doctests are meant to act as documentation as well as tests. When we start adding new test cases, they should be added as separate pytests.

>>> f = open('test_follow.txt', 'w')
>>> fo = open('test_follow.txt', 'r')
>>> generator = follow(fo)
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't seem to be testing the case of "when a file currently being opened is renamed" while following.

This is starting a new follow(fo) with the new file. What I would expect is the original generator is used without needed to create a new follow.

>>> _ = f.write('Line 1\\n')
>>> f.flush()
>>> next(generator)
'Line 1'
>>> f.close()
>>> fo.close()
>>> os.remove('test_follow.txt')
>>> os.remove('test_follow_1.txt')
"""
return Tailer(file, end=True).follow(delay)

Expand Down