[IO-247] IOUtils.closeQuietly should be documented or deprecated

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

Environment

All

• Assignee: Niall Pemberton
• Reporter: Al McDowell
• Created: 08/Aug/10 13:34
• Updated: 29/Dec/10 15:10
• Resolved: 08/Sep/10 17:00

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.