Commons IO
  1. Commons IO
  2. IO-269

Tailer locks file from deletion/rename on Windows

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 2.4
    • Component/s: None
    • Labels:
      None

      Description

      The Tailer code works on Windows, except that it locks the file against deletion or rename.

      The test code fails to detect this, because it fails to check if the file deletion succeeds.

      This seems to be a Windows OS issue.

      A possible solution might be to keep closing and re-opening the file.

      1. IO-269.patch
        5 kB
        Marcos Vinícius da Silva
      2. IO-269-v2.patch
        5 kB
        Niall Pemberton
      3. IO-269-v3.patch
        6 kB
        Sebb
      4. ReOpen.java
        2 kB
        Sebb

        Activity

        Hide
        Niall Pemberton added a comment -

        This was reported by Hemal Pandya in the following thread:

        http://markmail.org/message/3qydndh4doszjmdz

        Show
        Niall Pemberton added a comment - This was reported by Hemal Pandya in the following thread: http://markmail.org/message/3qydndh4doszjmdz
        Hide
        Marcos Vinícius da Silva added a comment -

        Attached a patch.
        I did a small manual benchmark here, comparing the old and new implementation, and, in fact, the new implementation is slight faster (0,1s). So keep reopening the file is not a problem.

        Show
        Marcos Vinícius da Silva added a comment - Attached a patch. I did a small manual benchmark here, comparing the old and new implementation, and, in fact, the new implementation is slight faster (0,1s). So keep reopening the file is not a problem.
        Hide
        Sebb added a comment -

        Thanks for the patch.

        In the benchmark, how big was the file that you were tailing?

        Show
        Sebb added a comment - Thanks for the patch. In the benchmark, how big was the file that you were tailing?
        Hide
        Marcos Vinícius da Silva added a comment -

        The file was 9,73Mb.
        The tailer was running with zero delay.

        Show
        Marcos Vinícius da Silva added a comment - The file was 9,73Mb. The tailer was running with zero delay.
        Hide
        Niall Pemberton added a comment -

        The original patch is now out-of-date. Attaching IO-269-v2.patch which is up-to-date with the current code base

        Show
        Niall Pemberton added a comment - The original patch is now out-of-date. Attaching IO-269 -v2.patch which is up-to-date with the current code base
        Hide
        Sebb added a comment -

        File locking is only a problem on Windows, and could perhaps be addressed in a later release of the JVM.
        Also, for some applications, it may not be necessary to allow concurrent rename/deletion.
        So I think the re-open strategy should be an optional feature.

        Show
        Sebb added a comment - File locking is only a problem on Windows, and could perhaps be addressed in a later release of the JVM. Also, for some applications, it may not be necessary to allow concurrent rename/deletion. So I think the re-open strategy should be an optional feature.
        Hide
        Niall Pemberton added a comment -

        With NIO2 in JDK 7 I wouldn't hold out much hope for future file improvements.

        Also optional platform specific features really go against the ethos of Java's write once, run anywhere.

        Any reason why reopening is a bad idea on other non-Windoze platforms?

        Show
        Niall Pemberton added a comment - With NIO2 in JDK 7 I wouldn't hold out much hope for future file improvements. Also optional platform specific features really go against the ethos of Java's write once, run anywhere. Any reason why reopening is a bad idea on other non-Windoze platforms?
        Hide
        Sebb added a comment -

        Also optional platform specific features really go against the ethos of Java's write once, run anywhere.

        I was not proposing making the code OS-specific.

        Any reason why reopening is a bad idea on other non-Windoze platforms?

        It would be somewhat less efficient.
        Also, AFAIK the Tailer can continue following a renamed file.

        If there do turn out to be issues with the new strategy - could it cause file rotations to be missed? - making it optional is safer.

        Show
        Sebb added a comment - Also optional platform specific features really go against the ethos of Java's write once, run anywhere. I was not proposing making the code OS-specific. Any reason why reopening is a bad idea on other non-Windoze platforms? It would be somewhat less efficient. Also, AFAIK the Tailer can continue following a renamed file. If there do turn out to be issues with the new strategy - could it cause file rotations to be missed? - making it optional is safer.
        Hide
        Niall Pemberton added a comment -

        Do you have any benchmark to back up the less efficient statement? Marcos indicated that it was not the case in his benchmark.

        Continuing to tail a file thats been renamed would be a bug (log files is a good example of that).

        The current implementation can miss rotations - thats a factor of the delay and how fast/frequently the file is being written to. So the question really is, is it more likely to miss rotations and I believe that goes back to the efficiency question.

        Show
        Niall Pemberton added a comment - Do you have any benchmark to back up the less efficient statement? Marcos indicated that it was not the case in his benchmark. Continuing to tail a file thats been renamed would be a bug (log files is a good example of that). The current implementation can miss rotations - thats a factor of the delay and how fast/frequently the file is being written to. So the question really is, is it more likely to miss rotations and I believe that goes back to the efficiency question.
        Hide
        Sebb added a comment -

        Marcos indicated that it was not the case in his benchmark.

        The readLines() method won't return until it reaches EOF, so when run against a pre-existing file there won't be any re-opens. It has to be run against a file that is growing.

        Do you have any benchmark to back up the less efficient statement?

        Yes, it can be up to about 45% slower.
        I used the attached benchmark (to follow), which simulates EOF by reading lines in chunks.

        Show
        Sebb added a comment - Marcos indicated that it was not the case in his benchmark. The readLines() method won't return until it reaches EOF, so when run against a pre-existing file there won't be any re-opens. It has to be run against a file that is growing. Do you have any benchmark to back up the less efficient statement? Yes, it can be up to about 45% slower. I used the attached benchmark (to follow), which simulates EOF by reading lines in chunks.
        Hide
        Sebb added a comment -

        Patch to optionally close input file whilst waiting.

        Also update test case so file deletion works on Windows

        Show
        Sebb added a comment - Patch to optionally close input file whilst waiting. Also update test case so file deletion works on Windows
        Hide
        Gary Gregory added a comment -

        Sending C:/svn/org/apache/commons/trunks-proper/io/src/changes/changes.xml
        Sending C:/svn/org/apache/commons/trunks-proper/io/src/main/java/org/apache/commons/io/input/Tailer.java
        Sending C:/svn/org/apache/commons/trunks-proper/io/src/test/java/org/apache/commons/io/input/TailerTest.java
        Transmitting file data ...
        Committed revision 1348698.

        Show
        Gary Gregory added a comment - Sending C:/svn/org/apache/commons/trunks-proper/io/src/changes/changes.xml Sending C:/svn/org/apache/commons/trunks-proper/io/src/main/java/org/apache/commons/io/input/Tailer.java Sending C:/svn/org/apache/commons/trunks-proper/io/src/test/java/org/apache/commons/io/input/TailerTest.java Transmitting file data ... Committed revision 1348698.
        Hide
        Robert Olofsson added a comment -

        I came across this issue and noticed an inefficiency in the code of the run method.

        With the reOpen flag set a new RandomAccessFile is always created at the end of the main while loop in the run method:

        if (getRun() && reOpen)

        { reader = new RandomAccessFile(file, RAF_MODE); reader.seek(position); }

        This is unnecessary and contributes to unnecessary file locking on Windows.

        If the reOpen flag is set a new RandomAccessFile should only be created when the length of the file or the last modification date indicate that the file needs to be read.

        Show
        Robert Olofsson added a comment - I came across this issue and noticed an inefficiency in the code of the run method. With the reOpen flag set a new RandomAccessFile is always created at the end of the main while loop in the run method: if (getRun() && reOpen) { reader = new RandomAccessFile(file, RAF_MODE); reader.seek(position); } This is unnecessary and contributes to unnecessary file locking on Windows. If the reOpen flag is set a new RandomAccessFile should only be created when the length of the file or the last modification date indicate that the file needs to be read.
        Hide
        Sebb added a comment -

        This is unnecessary and contributes to unnecessary file locking on Windows.

        On the contrary, it is an essential part of unlocking the file on Windows when possible.
        The full code is as follows:

        if (reOpen) {
            IOUtils.closeQuietly(reader);
        }
        Thread.sleep(delayMillis);
        if (getRun() && reOpen) {
            reader = new RandomAccessFile(file, RAF_MODE);
            reader.seek(position);
        }
        

        Notice that the file is closed - and therefore unlocked - during the sleep if reOpen is true.
        If reOpen is false, the file remains locked by Windows for the duration of the Tailer thread.

        Show
        Sebb added a comment - This is unnecessary and contributes to unnecessary file locking on Windows. On the contrary, it is an essential part of unlocking the file on Windows when possible. The full code is as follows: if (reOpen) { IOUtils.closeQuietly(reader); } Thread .sleep(delayMillis); if (getRun() && reOpen) { reader = new RandomAccessFile(file, RAF_MODE); reader.seek(position); } Notice that the file is closed - and therefore unlocked - during the sleep if reOpen is true. If reOpen is false, the file remains locked by Windows for the duration of the Tailer thread.
        Hide
        Robert Olofsson added a comment -

        Yes the file is closed when while loop sleeps, however what I'm trying to point out is that the file is always opened during each iteration of the loop even when it's not necessary.

        There is no need to open the file unless changes to it's length or last modified date are detected.

        I've created a modified version of Tailer for small desktop app called SwingTail (http://unlogic.se/projects/swingtail) the incorporates these changes among others. However I've done so many other changes that I couldn't generate a clean diff. But the source is available here:

        svn://svn.unlogic.se/swingtail/trunk/src/se/unlogic/swingtail/Tailer.java

        Essentially I simply removed the open statement from the end of the loop and added three separate open statements higher up the loop that only open the file when a change is actually detected.

        Show
        Robert Olofsson added a comment - Yes the file is closed when while loop sleeps, however what I'm trying to point out is that the file is always opened during each iteration of the loop even when it's not necessary. There is no need to open the file unless changes to it's length or last modified date are detected. I've created a modified version of Tailer for small desktop app called SwingTail ( http://unlogic.se/projects/swingtail ) the incorporates these changes among others. However I've done so many other changes that I couldn't generate a clean diff. But the source is available here: svn://svn.unlogic.se/swingtail/trunk/src/se/unlogic/swingtail/Tailer.java Essentially I simply removed the open statement from the end of the loop and added three separate open statements higher up the loop that only open the file when a change is actually detected.
        Hide
        Sebb added a comment -

        I see now.

        In that case, I think a new JIRA issue is required.
        The original issue was fixed so is complete.
        Your proposal is to improve the locking behaviour, so should be filed as an enhancement please.

        Show
        Sebb added a comment - I see now. In that case, I think a new JIRA issue is required. The original issue was fixed so is complete. Your proposal is to improve the locking behaviour, so should be filed as an enhancement please.

          People

          • Assignee:
            Unassigned
            Reporter:
            Sebb
          • Votes:
            2 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development