Commons IO
  1. Commons IO
  2. IO-247

IOUtils.closeQuietly should be documented or deprecated

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4
    • Fix Version/s: 2.0
    • Component/s: Utilities
    • Labels:
      None
    • Environment:

      All

      Description

      This is primarily a documentation and education issue.

      IOUtils.closeQuietly does exactly what is says on the tin, so the method isn't defective, but I frequently see it abused to write buggy code of this form:

      BadStreamHandling.java
      //don't write code like this
      
          byte[] data = "Hello, World".getBytes();
      
          OutputStream out = null;
          try {
            File temp = File.createTempFile(IOBug.class.getName(), ".txt");
            System.out.println(temp);
            out = new FileOutputStream(temp);
            out = new BufferedOutputStream(out);
            out.write(data);
          } catch (IOException e) {
            // error handling
          } finally {
            //errors writing buffered output are swallowed
            IOUtils.closeQuietly(out);
          }
      

      Search a site like stackoverflow.com for IOUtils.closeQuietly and half the examples are broken in some way. The contract for most closeable classes is that the close method can throw an IOException. By swallowing the exception, callers are not handling errors and this is rarely acceptable. In the case of output streams, this can lead to data loss.

      The value of a utility library is diminished if it just provides a faster way to write bad code.

      Proposed fix

      I'd like to see some advice added to the javadoc about how and when to use these methods - canonical examples wouldn't hurt.

      This example code would honour the class contract:

      BetterStreamHandling.java
      //better
      
          byte[] data = "Hello, World".getBytes();
      
          OutputStream out = null;
          try {
            File temp = File.createTempFile(IOBug.class.getName(), ".txt");
            System.out.println(temp);
            out = new FileOutputStream(temp);
            out = new BufferedOutputStream(out);
            out.write(data);
            out.close(); //close errors are handled
          } catch (IOException e) {
            // error handling
          } finally {
            IOUtils.closeQuietly(out);
          }
      

      Note: using flush is not an acceptable alternative. It is better to write to the abstraction and not the implementation and some stream types will not be able to commit all data until close is called anyway. I don't mean to lecture - I'm just trying to be thorough - this is another common piece of advice used in connection with these methods.

        Activity

        Al McDowell created issue -
        Niall Pemberton made changes -
        Field Original Value New Value
        Status Open [ 1 ] Resolved [ 5 ]
        Assignee Niall Pemberton [ niallp ]
        Fix Version/s 2.0 [ 12312961 ]
        Resolution Fixed [ 1 ]
        Al McDowell made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Mark Thomas made changes -
        Workflow jira [ 12517466 ] Default workflow, editable Closed status [ 12601902 ]

          People

          • Assignee:
            Niall Pemberton
            Reporter:
            Al McDowell
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Time Tracking

              Estimated:
              Original Estimate - 2h
              2h
              Remaining:
              Remaining Estimate - 2h
              2h
              Logged:
              Time Spent - Not Specified
              Not Specified

                Development