Commons IO
  1. Commons IO
  2. IO-303

TeeOutputStream does not call branch.close() when main.close() throws an exception

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.2
    • Component/s: Streams/Writers
    • Labels:

      Description

      TeeOutputStream.close() looks like this:

      TeeOutputStream.java
          /**
           * Closes both streams. 
           * @throws IOException if an I/O error occurs
           */
          @Override
          public void close() throws IOException {
              super.close();
              this.branch.close();
          }
      

      It is obvious that this.branch.close() is not executed when super.close() raises an exception. super.close() may in fact raise an IOException since ProxyOutputStream.handleIOException(IOException) is not overridden.

        Activity

        Hide
        Gary Gregory added a comment -

        YW.
        BTW, feel free to contribute patches and to improve the test coverage too!

        Show
        Gary Gregory added a comment - YW. BTW, feel free to contribute patches and to improve the test coverage too!
        Hide
        Gary Gregory added a comment -

        Looks reasonable, re-implemented in SVN as above.

        Show
        Gary Gregory added a comment - Looks reasonable, re-implemented in SVN as above.
        Hide
        Fabian Barney added a comment - - edited

        Thanks for addressing this issue that fast!
        Here's how I would write it but it is just a matter of taste...

            @Override
            public void close() throws IOException {        
                try {
                    super.close();
                } 
                finally {
                    this.branch.close();
                }
            }
        
        Show
        Fabian Barney added a comment - - edited Thanks for addressing this issue that fast! Here's how I would write it but it is just a matter of taste... @Override public void close() throws IOException { try { super.close(); } finally { this.branch.close(); } }
        Hide
        Gary Gregory added a comment -

        This is the solution I implemented. For simplicity's sake, note that the exception thrown is the last exception encountered.

            /**
             * Closes both output streams.
             * 
             * If closing the main output stream throws an exception, attempt to close the branch output stream.
             * 
             * If closing the main and branch output streams both throw exceptions, which exceptions is thrown by this method is
             * currently unspecified and subject to change.
             * 
             * @throws IOException
             *             if an I/O error occurs
             */
            @Override
            public void close() throws IOException {        
                try {
                    super.close();
                } catch (IOException e) {
                    this.branch.close();
                    throw e;
                }
                this.branch.close();
            }
        
        Show
        Gary Gregory added a comment - This is the solution I implemented. For simplicity's sake, note that the exception thrown is the last exception encountered. /** * Closes both output streams. * * If closing the main output stream throws an exception, attempt to close the branch output stream. * * If closing the main and branch output streams both throw exceptions, which exceptions is thrown by this method is * currently unspecified and subject to change. * * @ throws IOException * if an I/O error occurs */ @Override public void close() throws IOException { try { super .close(); } catch (IOException e) { this .branch.close(); throw e; } this .branch.close(); }
        Hide
        Fabian Barney added a comment - - edited

        In my code I prefer throwing the first one. There is one exception when a latter Throwable occurrs and it is an Error and the former not.
        In my opinion this is the Throwable you want to see.

        Another approach is to throw something like a MultiIOException containing all occurred exceptions.

        I agree that this all is not a real pleasure, but better than leaving resources open that can be closed successfully.
        I've written a MultiOutputStream yesterday: https://github.com/fabian-barney/Utils/blob/master/utils/src/com/barney4j/utils/io/MultiOutputStream.java

        I am not sure for myself that I made the right decision here.

        Show
        Fabian Barney added a comment - - edited In my code I prefer throwing the first one. There is one exception when a latter Throwable occurrs and it is an Error and the former not. In my opinion this is the Throwable you want to see. Another approach is to throw something like a MultiIOException containing all occurred exceptions. I agree that this all is not a real pleasure, but better than leaving resources open that can be closed successfully. I've written a MultiOutputStream yesterday: https://github.com/fabian-barney/Utils/blob/master/utils/src/com/barney4j/utils/io/MultiOutputStream.java I am not sure for myself that I made the right decision here.
        Hide
        Gary Gregory added a comment -

        If both close() methods throw an exception, which exception does the method throw? I am guessing the first.

        Show
        Gary Gregory added a comment - If both close() methods throw an exception, which exception does the method throw? I am guessing the first.

          People

          • Assignee:
            Gary Gregory
            Reporter:
            Fabian Barney
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development