java 6 IO - wrapped streams closing [closed]

Question

Consider :

public static void read(String filename) throws IOException {
    String charsetName = "UTF-8";
    InputStream file = new FileInputStream(filename); // say no problem
    InputStreamReader reader = new InputStreamReader(file, charsetName);
    BufferedReader buffer = new BufferedReader(reader);
    try {
        buffer.readLine();
    } finally {
        try {
            buffer.close();
        } catch (IOException e) {
            // report at least
            e.printStackTrace();
        }
    }
}

If new InputStreamReader(file, charsetName) throws UnsupportedEncodingException, the buffer.close(); line will never be called. The alternative is extra verbose :

InputStream file = new FileInputStream(filename);
try {
    InputStreamReader reader = new InputStreamReader(file);
    try {
        BufferedReader buffer = new BufferedReader(buffer);
        try {
            buffer.readLine();
        } finally {
            buffer.close(); // should catch
        }
    } finally {
        reader.close(); // should catch
    }
} finally {
    file.close(); // should catch
}

and it needlessly closes all streams (while output.close(); should suffice - actually any of them should suffice if successful - see comments in the code by Skeet).

Wrapping the constructors

BufferedReader buffer = new BufferedReader(
              new InputStreamReader(new FileInputStream(filename), charsetName));

is essentially just hiding the problem.

Notice I use the try-finally idiom suggested by @TomHawtin-tackline here for instance - but the more common approach :

public static void read(String filename) throws IOException {
    String charsetName = "UTF-8";
    InputStream file = null;
    InputStreamReader reader = null;
    BufferedReader buffer = null;
    try {
        file = new FileInputStream(filename);
        reader = new InputStreamReader(file, charsetName);
        buffer = new BufferedReader(reader);
        buffer.readLine();
    } finally {
        try {
            if(buffer != null) buffer.close();
        } catch (IOException e) {
            // report at least
            e.printStackTrace();
        }
        // Rinse and repeat for the rest
    }
}

is as awkward.

Question :

How would you handle this case ?
Would :

public static void read(String filename) throws IOException {
    String charsetName = "UTF-8";
    InputStream file = new FileInputStream(filename);
    try {
        InputStreamReader reader = new InputStreamReader(file, charsetName);
        BufferedReader buffer = new BufferedReader(reader); // Eclipse warning
        buffer.readLine();
        // notice that if these were out put streams we SHOULD FLUSH HERE
    } finally {
        try {
            file.close();
        } catch (IOException e) {
            // report at least
            e.printStackTrace();
        }
    }
}

do ? In other words closing the innermost stream (contrary to what is usually asked) would be the cleanest solution when there are more than 2 wrapped streams ? Are there cases when finally the decorator should be also closed() ? See for instance points here. Notice that Eclipse warns :

Resource leak: 'buffer' is never closed

Is Eclipse right ?

_{This is Java 6 - Android is Java 6 only I remind you. Trying to factor out IO code in some utility classes, once and for all}

Coalfield

@Vishal : what is awkward is the code in the second and 4rth code snippets not Thomas' suggestion- read carefully – Coalfield

OK..updated the comment. – Breadth

I saw Your fourth snippet of code..And correct me if I am wrong By 4rth snippet you are referring to the code containing following line in catch block if(buffer != null) buffer.close();..right? – Breadth

@VishalK: Yes - what I mean is that if there are more than 2 streams (whose constructors may throw) both ways are awkward - so I ask how should one handle this elegantly. My way of doing it (closing the innermost stream) has the disadvantages 1 of not flushing the streams (for output streams not in the code I posted) 2 of not closing potential decorators that need to be closed and 3 of generating warnings - so what would be the way to do it not awkwardly ? – Coalfield

Answer 1

In your last approach , Resource leak warning as shown by eclipse is correct according to the rules of closing streams. closing the innermost stream is only closing that stream but not the other streams that has wrapped up that stream. But closing the outermost stream is one time operation that will automatically close all the underlying streams. As specified in the article Always close streams :

If multiple streams are chained together, then closing the one which was the last to be constructed, and is thus at the highest level of abstraction, will automatically close all the underlying streams. So, one only has to call close on one stream in order to close (and flush, if applicable) an entire series of related streams.

So In my opinion following code is solution for your case:

public static void read(String filename) throws IOException {
    String charsetName = "UTF-8";
    InputStream file = null;
    InputStreamReader reader = null;
    BufferedReader buffer = null;
    try 
    {
        file = new FileInputStream(filename);                
        reader = new InputStream(file,charsetName);
        buffer = new BufferedReader(reader);
        buffer.readLine();
    } 
    finally 
    {
         closeQuietly(buffer,reader,file);
    }
}

EDIT
As suggested by @jtahlborn the code written within finally block is wrapped within a utility method as follows:

public static void closeQuietly(Closeable... closeables) 
{ 
    if(closeables == null) 
    { 
        return; 
    } 
    for (Closeable c : closeables) 
    { 
        doCloseQuietly(c); 
    } 
} 
public static void doCloseQuietly(Closeable c)
{
    try
    {
        if (c != null)
        {
            c.close();
        }
    }
    catch (IOException ex)
    {
        ex.printStackTrace();
    }
}

Breadth

i would do basically this with a utility method for closing with the signature closeQuietly(Closeable... res). so your finally block is just closeQuietly(buffer, reader, file). (multiple close calls isn't really an issue) – Synovia

@jtahlborn: yeah you are right. But IMO if this is one time operation in the whole program then creating a utility method is not worth in this case.. – Breadth

rarely is closing resources a one time thing, which is why you make this a utility method you can use anywhere (which we have at my workplace). – Synovia

@Coalfield - um, no, this code does not suffer from the same bug as your first snippet. please read a little more carefully before yelling incorrect things in your comments. – Synovia

@Coalfield - on the last line of the finally block (assuming the typo was corrected). i assume the last lines are supposed to reference the file var not buffer (again). – Synovia

@Mr_and_Mrs_D: Yeah according to jtahlborn it was typo mistake which I had rectified now.. And this code is not resembling any of the code snippets that you have posted..I don't see any resource leak here.. – Breadth

@Coalfield - i proposed an addition in my comment above which would make the code a bit more "elegant". – Synovia

@Coalfield - first of all, you can drop the attitude. second of all, you make no reference to such a utility method in your post that i can see... at this point, i fail to see any useful point to your question. – Synovia

@Mr_and_Mrs_D: How you define elegant solution for this case? If one event fails then other has to occur..if it fails then next..This is how the streams could be closed gracefully..You CAN'T avoid this... And regarding the way you are responding to comments that is not elegant itself..If you keep this attitude then I don't think you are going to get answers for your questions at least in SO..... – Breadth

@VishalK - that's not the type of utility method i had in mind. that is specific to this use and not re-usable. – Synovia

@jtahlborn: thanks for ur feedback..Can I know the prototype of utility method that you had thought of? – Breadth

public static void closeQuietly(Closeable... closeables) { if(closeables == null) { return; } for (Closeable c : closeables) { try { if (c != null) { c.close(); } } catch (IOException e) { } } } – Synovia

@jtahlborn: Thanks ..incorporated that.. – Breadth

Answer 2

The second pattern (the check for null one) could be amended as :

public static void readSO2(String filename) throws IOException {
    String charsetName = "UTF-8";
    InputStream file = null;
    InputStreamReader reader = null;
    BufferedReader buffer = null;
    try {
        file = new FileInputStream(filename);
        reader = new InputStreamReader(file, charsetName);
        buffer = new BufferedReader(reader);
        buffer.readLine();
    } finally {
        try {
            if (buffer != null) buffer.close();
            else if (reader != null) reader.close();
            else if (file != null) file.close();
        } catch (IOException e) {
            // report at least
            e.printStackTrace();
        }
    }
}

• Pros : one close, one possible exception, no flush needed (for output streams)
• Cons : still inelegant : null dance, one must be careful in the order of the ifs, no way to generalize

So question still stands.

Coalfield