Commons IO
  1. Commons IO
  2. IO-357

[Tailer] InterruptedException while the thead is sleeping is silently ignored

    Details

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

      Description

      The implementation of org.apache.commons.io.input.Tailer silently ignores interruptions while sleeping (in two places).

      Source snippet:

      360	                    try {
      361	                        Thread.sleep(delayMillis);
      362	                    } catch (InterruptedException e) {
      363	                    }
      ...
      425	                try {
      426	                    Thread.sleep(delayMillis);
      427	                } catch (InterruptedException e) {
      428	                }
      

      This is an inappropriate behavior, since it prevents controlled shutdown by a container.

      This may be rectified in one of these ways:

      1. Declare the method as "throws InterruptedException" and re-throw the InterruptedException, after possibly performing come cleanup, or removing the catch clause entirely. This will ensure that a thread interruption (possibly caused by the forced shutdown by a container) will cause processing to stop, and shutdown to proceed. Problem: Requires backwards incompatible change to method signature.
      2. Treat an interrupt as an alternate way of signalling the Tailer to stop, by calling stop() in the catch clause.
      3. Reassert the interrupted state of the thread by calling Thread.currentThread.interrupt() to be able to detect the interruption at a later stage.

      For reference, please refer to these resources about handling thread interruption:

        Activity

        Hide
        Gary Gregory added a comment -
        commit -m "[IO-358][Tailer] InterruptedException while the thread is sleeping is silently ignored." C:/svn/org/apache/commons/trunks-proper/io/src/test/java/org/apache/commons/io/input/TailerTest.java C:/svn/org/apache/commons/trunks-proper/io/src/main/java/org/apache/commons/io/input/Tailer.java C:/svn/org/apache/commons/trunks-proper/io/src/changes/changes.xml
            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 1412391.
        
        Show
        Gary Gregory added a comment - commit -m "[IO-358][Tailer] InterruptedException while the thread is sleeping is silently ignored." C:/svn/org/apache/commons/trunks-proper/io/src/test/java/org/apache/commons/io/input/TailerTest.java C:/svn/org/apache/commons/trunks-proper/io/src/main/java/org/apache/commons/io/input/Tailer.java C:/svn/org/apache/commons/trunks-proper/io/src/changes/changes.xml 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 1412391.
        Hide
        Gary Gregory added a comment -

        Or like this:

        Index: src/main/java/org/apache/commons/io/input/Tailer.java
        ===================================================================
        --- src/main/java/org/apache/commons/io/input/Tailer.java	(revision 1411980)
        +++ src/main/java/org/apache/commons/io/input/Tailer.java	(working copy)
        @@ -356,10 +356,7 @@
                             listener.fileNotFound();
                         }
                         if (reader == null) {
        -                    try {
        -                        Thread.sleep(delayMillis);
        -                    } catch (InterruptedException e) {
        -                    }
        +                    Thread.sleep(delayMillis);
                         } else {
                             // The current position in the file
                             position = end ? file.length() : 0;
        @@ -410,22 +407,27 @@
                         if (reOpen) {
                             IOUtils.closeQuietly(reader);
                         }
        -                try {
        -                    Thread.sleep(delayMillis);
        -                } catch (InterruptedException e) {
        -                }
        +                Thread.sleep(delayMillis);
                         if (getRun() && reOpen) {
                             reader = new RandomAccessFile(file, RAF_MODE);
                             reader.seek(position);
                         }
                     }
        -        } catch (Exception e) {
        -            listener.handle(e);
        +        } catch (InterruptedException e) {            
        +            Thread.currentThread().interrupt();
        +            stop(e);
        +        } catch (Exception e) {            
        +            stop(e);
                 } finally {
                     IOUtils.closeQuietly(reader);
                 }
             }
         
        +    private void stop(Exception e) {
        +        listener.handle(e);
        +        stop();
        +    }
        +
             /**
              * Allows the tailer to complete its current loop and return.
              */
        
        Show
        Gary Gregory added a comment - Or like this: Index: src/main/java/org/apache/commons/io/input/Tailer.java =================================================================== --- src/main/java/org/apache/commons/io/input/Tailer.java (revision 1411980) +++ src/main/java/org/apache/commons/io/input/Tailer.java (working copy) @@ -356,10 +356,7 @@ listener.fileNotFound(); } if (reader == null ) { - try { - Thread .sleep(delayMillis); - } catch (InterruptedException e) { - } + Thread .sleep(delayMillis); } else { // The current position in the file position = end ? file.length() : 0; @@ -410,22 +407,27 @@ if (reOpen) { IOUtils.closeQuietly(reader); } - try { - Thread .sleep(delayMillis); - } catch (InterruptedException e) { - } + Thread .sleep(delayMillis); if (getRun() && reOpen) { reader = new RandomAccessFile(file, RAF_MODE); reader.seek(position); } } - } catch (Exception e) { - listener.handle(e); + } catch (InterruptedException e) { + Thread .currentThread().interrupt(); + stop(e); + } catch (Exception e) { + stop(e); } finally { IOUtils.closeQuietly(reader); } } + private void stop(Exception e) { + listener.handle(e); + stop(); + } + /** * Allows the tailer to complete its current loop and return . */
        Hide
        Charles Honton added a comment -

        InterruptedException should reset the interrupt condition before calling listener.handle(e).

        -        } catch (Exception e) {
        +        } catch (InterruptedException e) {            
        +            Thread.currentThread().interrupt();
                     listener.handle(e);
        +        } catch (Exception e) {            
        +            listener.handle(e);
                 } finally {
                     IOUtils.closeQuietly(reader);
                 }
        

        or

                 } catch (Exception e) {
        +            if(e instanceof InterruptedException) {            
        +                Thread.currentThread().interrupt();
        +            }
                     listener.handle(e);
                 } finally {
                     IOUtils.closeQuietly(reader);
                 }
        
        Show
        Charles Honton added a comment - InterruptedException should reset the interrupt condition before calling listener.handle(e). - } catch (Exception e) { + } catch (InterruptedException e) { + Thread .currentThread().interrupt(); listener.handle(e); + } catch (Exception e) { + listener.handle(e); } finally { IOUtils.closeQuietly(reader); } or } catch (Exception e) { + if (e instanceof InterruptedException) { + Thread .currentThread().interrupt(); + } listener.handle(e); } finally { IOUtils.closeQuietly(reader); }
        Hide
        Gary Gregory added a comment -

        "AFAICT there's no point calling stop outside the run loop, because the flag won't be checked."

        I added a call to stop() to play nice with IO-345: Supply a hook method allowing Tailer actively determining stop condition.

        This lets a Tailer subclass (as requested in IO-345) access the run value.

        "Also InterruptedException is an instance of Exception; no point in having a separate catch unless the code is different."

        The code in the two catche clauses is different. For InterruptedException, interrupt() is called.

        It is more obvious like this:

        Index: src/main/java/org/apache/commons/io/input/Tailer.java
        ===================================================================
        --- src/main/java/org/apache/commons/io/input/Tailer.java	(revision 1411980)
        +++ src/main/java/org/apache/commons/io/input/Tailer.java	(working copy)
        @@ -356,10 +356,7 @@
                             listener.fileNotFound();
                         }
                         if (reader == null) {
        -                    try {
        -                        Thread.sleep(delayMillis);
        -                    } catch (InterruptedException e) {
        -                    }
        +                    Thread.sleep(delayMillis);
                         } else {
                             // The current position in the file
                             position = end ? file.length() : 0;
        @@ -410,22 +407,27 @@
                         if (reOpen) {
                             IOUtils.closeQuietly(reader);
                         }
        -                try {
        -                    Thread.sleep(delayMillis);
        -                } catch (InterruptedException e) {
        -                }
        +                Thread.sleep(delayMillis);
                         if (getRun() && reOpen) {
                             reader = new RandomAccessFile(file, RAF_MODE);
                             reader.seek(position);
                         }
                     }
        -        } catch (Exception e) {
        -            listener.handle(e);
        +        } catch (InterruptedException e) {            
        +            stop(e);
        +            Thread.currentThread().interrupt();
        +        } catch (Exception e) {            
        +            stop(e);
                 } finally {
                     IOUtils.closeQuietly(reader);
                 }
             }
         
        +    private void stop(Exception e) {
        +        listener.handle(e);
        +        stop();
        +    }
        +
             /**
              * Allows the tailer to complete its current loop and return.
              */
        
        Show
        Gary Gregory added a comment - "AFAICT there's no point calling stop outside the run loop, because the flag won't be checked." I added a call to stop() to play nice with IO-345 : Supply a hook method allowing Tailer actively determining stop condition. This lets a Tailer subclass (as requested in IO-345 ) access the run value. "Also InterruptedException is an instance of Exception; no point in having a separate catch unless the code is different." The code in the two catche clauses is different. For InterruptedException, interrupt() is called. It is more obvious like this: Index: src/main/java/org/apache/commons/io/input/Tailer.java =================================================================== --- src/main/java/org/apache/commons/io/input/Tailer.java (revision 1411980) +++ src/main/java/org/apache/commons/io/input/Tailer.java (working copy) @@ -356,10 +356,7 @@ listener.fileNotFound(); } if (reader == null) { - try { - Thread.sleep(delayMillis); - } catch (InterruptedException e) { - } + Thread.sleep(delayMillis); } else { // The current position in the file position = end ? file.length() : 0; @@ -410,22 +407,27 @@ if (reOpen) { IOUtils.closeQuietly(reader); } - try { - Thread.sleep(delayMillis); - } catch (InterruptedException e) { - } + Thread.sleep(delayMillis); if (getRun() && reOpen) { reader = new RandomAccessFile(file, RAF_MODE); reader.seek(position); } } - } catch (Exception e) { - listener.handle(e); + } catch (InterruptedException e) { + stop(e); + Thread.currentThread().interrupt(); + } catch (Exception e) { + stop(e); } finally { IOUtils.closeQuietly(reader); } } + private void stop(Exception e) { + listener.handle(e); + stop(); + } + /** * Allows the tailer to complete its current loop and return. */
        Hide
        Gary Gregory added a comment -

        "AFAICT there's no point calling stop outside the run loop, because the flag won't be checked."

        I added a call to stop() to play nice with IO-345: Supply a hook method allowing Tailer actively determining stop condition.

        This lets a Tailer subclass (as requested in IO-345) access the run value.

        "Also InterruptedException is an instance of Exception; no point in having a separate catch unless the code is different."

        The code in the two catche clauses is different. For InterruptedException, interrupt() is called.

        It is more obvious like this:

        Index: src/main/java/org/apache/commons/io/input/Tailer.java
        ===================================================================
        --- src/main/java/org/apache/commons/io/input/Tailer.java	(revision 1411980)
        +++ src/main/java/org/apache/commons/io/input/Tailer.java	(working copy)
        @@ -356,10 +356,7 @@
                             listener.fileNotFound();
                         }
                         if (reader == null) {
        -                    try {
        -                        Thread.sleep(delayMillis);
        -                    } catch (InterruptedException e) {
        -                    }
        +                    Thread.sleep(delayMillis);
                         } else {
                             // The current position in the file
                             position = end ? file.length() : 0;
        @@ -410,22 +407,27 @@
                         if (reOpen) {
                             IOUtils.closeQuietly(reader);
                         }
        -                try {
        -                    Thread.sleep(delayMillis);
        -                } catch (InterruptedException e) {
        -                }
        +                Thread.sleep(delayMillis);
                         if (getRun() && reOpen) {
                             reader = new RandomAccessFile(file, RAF_MODE);
                             reader.seek(position);
                         }
                     }
        -        } catch (Exception e) {
        -            listener.handle(e);
        +        } catch (InterruptedException e) {            
        +            stop(e);
        +            Thread.currentThread().interrupt();
        +        } catch (Exception e) {            
        +            stop(e);
                 } finally {
                     IOUtils.closeQuietly(reader);
                 }
             }
         
        +    private void stop(Exception e) {
        +        listener.handle(e);
        +        stop();
        +    }
        +
             /**
              * Allows the tailer to complete its current loop and return.
              */
        
        Show
        Gary Gregory added a comment - "AFAICT there's no point calling stop outside the run loop, because the flag won't be checked." I added a call to stop() to play nice with IO-345 : Supply a hook method allowing Tailer actively determining stop condition. This lets a Tailer subclass (as requested in IO-345 ) access the run value. "Also InterruptedException is an instance of Exception; no point in having a separate catch unless the code is different." The code in the two catche clauses is different. For InterruptedException, interrupt() is called. It is more obvious like this: Index: src/main/java/org/apache/commons/io/input/Tailer.java =================================================================== --- src/main/java/org/apache/commons/io/input/Tailer.java (revision 1411980) +++ src/main/java/org/apache/commons/io/input/Tailer.java (working copy) @@ -356,10 +356,7 @@ listener.fileNotFound(); } if (reader == null) { - try { - Thread.sleep(delayMillis); - } catch (InterruptedException e) { - } + Thread.sleep(delayMillis); } else { // The current position in the file position = end ? file.length() : 0; @@ -410,22 +407,27 @@ if (reOpen) { IOUtils.closeQuietly(reader); } - try { - Thread.sleep(delayMillis); - } catch (InterruptedException e) { - } + Thread.sleep(delayMillis); if (getRun() && reOpen) { reader = new RandomAccessFile(file, RAF_MODE); reader.seek(position); } } - } catch (Exception e) { - listener.handle(e); + } catch (InterruptedException e) { + stop(e); + Thread.currentThread().interrupt(); + } catch (Exception e) { + stop(e); } finally { IOUtils.closeQuietly(reader); } } + private void stop(Exception e) { + listener.handle(e); + stop(); + } + /** * Allows the tailer to complete its current loop and return. */
        Hide
        Sebb added a comment -

        AFAICT there's no point calling stop outside the run loop, because the flag won't be checked.

        Also InterruptedException is an instance of Exception; no point in having a separate catch unless the code is different.

        So in fact the only change required would be to remove the catch blocks after Thread.sleep().

        Show
        Sebb added a comment - AFAICT there's no point calling stop outside the run loop, because the flag won't be checked. Also InterruptedException is an instance of Exception; no point in having a separate catch unless the code is different. So in fact the only change required would be to remove the catch blocks after Thread.sleep().
        Hide
        Morten Hattesen added a comment -

        Looks good to me!

        Show
        Morten Hattesen added a comment - Looks good to me!
        Hide
        Gary Gregory added a comment -

        Hm, I like the simplicity of handing the IE at the higher level like this (and also letting a user be able to find out about the IE):

        Index: src/main/java/org/apache/commons/io/input/Tailer.java
        ===================================================================
        --- src/main/java/org/apache/commons/io/input/Tailer.java	(revision 1411980)
        +++ src/main/java/org/apache/commons/io/input/Tailer.java	(working copy)
        @@ -356,10 +356,7 @@
                             listener.fileNotFound();
                         }
                         if (reader == null) {
        -                    try {
        -                        Thread.sleep(delayMillis);
        -                    } catch (InterruptedException e) {
        -                    }
        +                    Thread.sleep(delayMillis);
                         } else {
                             // The current position in the file
                             position = end ? file.length() : 0;
        @@ -410,17 +407,19 @@
                         if (reOpen) {
                             IOUtils.closeQuietly(reader);
                         }
        -                try {
        -                    Thread.sleep(delayMillis);
        -                } catch (InterruptedException e) {
        -                }
        +                Thread.sleep(delayMillis);
                         if (getRun() && reOpen) {
                             reader = new RandomAccessFile(file, RAF_MODE);
                             reader.seek(position);
                         }
                     }
        -        } catch (Exception e) {
        +        } catch (InterruptedException e) {            
                     listener.handle(e);
        +            stop();
        +            Thread.currentThread().interrupt();
        +        } catch (Exception e) {            
        +            listener.handle(e);
        +            stop();
                 } finally {
                     IOUtils.closeQuietly(reader);
                 }
        

        This based on the latest from trunk.

        Show
        Gary Gregory added a comment - Hm, I like the simplicity of handing the IE at the higher level like this (and also letting a user be able to find out about the IE): Index: src/main/java/org/apache/commons/io/input/Tailer.java =================================================================== --- src/main/java/org/apache/commons/io/input/Tailer.java (revision 1411980) +++ src/main/java/org/apache/commons/io/input/Tailer.java (working copy) @@ -356,10 +356,7 @@ listener.fileNotFound(); } if (reader == null) { - try { - Thread.sleep(delayMillis); - } catch (InterruptedException e) { - } + Thread.sleep(delayMillis); } else { // The current position in the file position = end ? file.length() : 0; @@ -410,17 +407,19 @@ if (reOpen) { IOUtils.closeQuietly(reader); } - try { - Thread.sleep(delayMillis); - } catch (InterruptedException e) { - } + Thread.sleep(delayMillis); if (getRun() && reOpen) { reader = new RandomAccessFile(file, RAF_MODE); reader.seek(position); } } - } catch (Exception e) { + } catch (InterruptedException e) { listener.handle(e); + stop(); + Thread.currentThread().interrupt(); + } catch (Exception e) { + listener.handle(e); + stop(); } finally { IOUtils.closeQuietly(reader); } This based on the latest from trunk.
        Hide
        Charles Honton added a comment -

        To allow calling Threads to know Tailer was stopped by interruption, use both options 2 and 3 from above:

        @@ -360,6 +360,7 @@
        try

        { Thread.sleep(delayMillis); }

        catch (InterruptedException e)

        { + stop(); + Thread.currentThread().interrupt(); }

        } else {
        // The current position in the file
        @@ -425,6 +426,7 @@
        try

        { Thread.sleep(delayMillis); }

        catch (InterruptedException e)

        { + stop(); + Thread.currentThread().interrupt(); }
        Show
        Charles Honton added a comment - To allow calling Threads to know Tailer was stopped by interruption, use both options 2 and 3 from above: @@ -360,6 +360,7 @@ try { Thread.sleep(delayMillis); } catch (InterruptedException e) { + stop(); + Thread.currentThread().interrupt(); } } else { // The current position in the file @@ -425,6 +426,7 @@ try { Thread.sleep(delayMillis); } catch (InterruptedException e) { + stop(); + Thread.currentThread().interrupt(); }
        Hide
        Morten Hattesen added a comment - - edited

        Having had another look at the code, you might also consider simply removing the two InterruptedException catch blocks, letting the surrounding catch block handle the InterruptException. That would exit the while loop, not requiring a call to stop(), and notifying the listener about the interruption:

        435	        } catch (Exception e) {
        436	            // Handles InterruptedException, too
        437	            listener.handle(e);
        438	
        439	        } finally {
        440	            IOUtils.closeQuietly(reader);
        441	        }
        

        As always, the code is complete, when there is no more code to be removed

        Show
        Morten Hattesen added a comment - - edited Having had another look at the code, you might also consider simply removing the two InterruptedException catch blocks, letting the surrounding catch block handle the InterruptException . That would exit the while loop, not requiring a call to stop() , and notifying the listener about the interruption: 435 } catch (Exception e) { 436 // Handles InterruptedException, too 437 listener.handle(e); 438 439 } finally { 440 IOUtils.closeQuietly(reader); 441 } As always, the code is complete, when there is no more code to be removed
        Hide
        Morten Hattesen added a comment -

        That would definitely do the job.

        The only minor issue would be that the calling thread would have no way of knowing that the Tailer was stopped by interruption, but I honestly don't see any situation where that would cause a major issue at all.

        It should be considered to also call listener.handle(e) prior to calling stop(), which would at least give the listener a chance to discover the interruption, and possibly perform some logging etc.

        Show
        Morten Hattesen added a comment - That would definitely do the job. The only minor issue would be that the calling thread would have no way of knowing that the Tailer was stopped by interruption, but I honestly don't see any situation where that would cause a major issue at all. It should be considered to also call listener.handle(e) prior to calling stop() , which would at least give the listener a chance to discover the interruption, and possibly perform some logging etc.
        Hide
        Gary Gregory added a comment -

        Well, (2) sounds pretty good.

        Any other thoughts?

        Index: src/main/java/org/apache/commons/io/input/Tailer.java
        ===================================================================
        --- src/main/java/org/apache/commons/io/input/Tailer.java	(revision 1402268)
        +++ src/main/java/org/apache/commons/io/input/Tailer.java	(working copy)
        @@ -360,6 +360,7 @@
                             try {
                                 Thread.sleep(delayMillis);
                             } catch (InterruptedException e) {
        +                        stop();
                             }
                         } else {
                             // The current position in the file
        @@ -425,6 +426,7 @@
                         try {
                             Thread.sleep(delayMillis);
                         } catch (InterruptedException e) {
        +                    stop();
                         }
                         if (getRun() && reOpen) {
                             reader = new RandomAccessFile(file, RAF_MODE);
        
        Show
        Gary Gregory added a comment - Well, (2) sounds pretty good. Any other thoughts? Index: src/main/java/org/apache/commons/io/input/Tailer.java =================================================================== --- src/main/java/org/apache/commons/io/input/Tailer.java (revision 1402268) +++ src/main/java/org/apache/commons/io/input/Tailer.java (working copy) @@ -360,6 +360,7 @@ try { Thread.sleep(delayMillis); } catch (InterruptedException e) { + stop(); } } else { // The current position in the file @@ -425,6 +426,7 @@ try { Thread.sleep(delayMillis); } catch (InterruptedException e) { + stop(); } if (getRun() && reOpen) { reader = new RandomAccessFile(file, RAF_MODE);

          People

          • Assignee:
            Unassigned
            Reporter:
            Morten Hattesen
          • Votes:
            0 Vote for this issue
            Watchers:
            4 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development