Java resource closing

I'm writing an app that connect to a website and read one line from it. I do it like this:

try{
        URLConnection connection = new URL("www.example.com").openConnection();
        BufferedReader rd = new BufferedReader(new InputStreamReader(connection.getInputStream()));
        String response = rd.readLine();
        rd.close();
    }catch (Exception e) {
        //exception handling
    }

Is it good? I mean, I close the BufferedReader in the last line, but I do not close the InputStreamReader. Should I create a standalone InputStreamReader from the connection.getInputStream, and a BufferedRead开发者_开发知识库er from the standalone InputStreamReader, than close all the two readers? I think it will be better to place the closing methods in the finally block like this:

InputStreamReader isr = null;
BufferedReader br = null;
try{
    URLConnection connection = new URL("www.example.com").openConnection();
    isr = new InputStreamReader(connection.getInputStream());
    br = new BufferedReader(isr);
    String response = br.readLine();
}catch (Exception e) {
    //exception handling
}finally{
    br.close();
    isr.close();
}

But it is ugly, because the closing methods can throw exception, so I have to handle or throw it.

Which solution is better? Or what would be the best solution?

The general idiom for resource acquisition and release in Java is:

final Resource resource = acquire();
try {
    use(resource);
} finally {
    resource.release();
}

Note:

• try should immediately follow the acquire. This means you can't wrap it in the decorator and maintain safety (and removing spaces or putting things on one line doesn't help:).
• One release per finally, otherwise it wont be exception safe.
• Avoid null, use final. Otherwise you'll have messy code and potential for NPEs.
• Generally there is no need to close the decorator unless it has a further resource associated with it. However, you will generally need to flush outputs, but avoid that in the exception case.
• The exception should either be passed through to the caller, or caught from a surrounding try block (Java leads you astray here).

ou can abstract this nonsense with the Execute Around idiom, so you don't have to repeat yourself (just write a lot of boilerplate).

Closing the BufferedReader is enough - this closes the underlying reader too.

Yishai posted a nice pattern for closing the streams (closing might throw another exception).

Is it good? I mean, I close the BufferedReader in the last line, but I do not close the InputStreamReader.

Apart from the fact that it should be done in the finally (so that the close is ensured, even in case of an exception), it's fine. The Java IO classes uses the decorator pattern. The close will be delegated to the underlying streams.

But it is ugly, because the closing methods can throw exception, so I have to handle or throw it.

When the close throws an exception, it often just means that the other side has been closed or deleted, which is completely out of your control. You can at highest log or ignore it. In a simple application I would just ignore it. In a mission critical application I would log it, just to be sure.

In a nut, your code can be rewritten as:

BufferedReader br = null;
try {
    URLConnection connection = new URL("www.example.com").openConnection();
    br = new BufferedReader(new InputStreamReader(connection.getInputStream()));
    String response = br.readLine();
}catch (Exception e) {
    //exception handling
}finally{
    if (br != null) try { br.close(); } catch (IOException ignore) {}
}

In Java 7 there will be automatic resource handling which would made your code as concise as:

try (BufferedReader br = new InputStreamReader(new URL("www.example.com").openStream())) {
    String response = br.readLine();
} catch (Exception e) {
    //exception handling
}

See also:

• Java IO tutorial
• C# "using" keyword in Java
• How to use URLConnection

BufferedReader br = null;

You are declaring a variable without assigning it (null doesn't count - it is a useless assignment in this case). This is a code "smell" in Java (ref Effective Java; Code Complete for more on variable declaration).

}finally{
    br.close();
    isr.close();
}

First, you only need to close the top-most stream decorator (br will close isr). Secondly, if br.close() threw an exception, isr.close() would not be called, so this is not sound code. Under certain exception conditions, your code will hide the originating exception with a NullPointerException.

isr = new InputStreamReader(connection.getInputStream());

If the (admittedly unlikely) event that the InputStreamReader constructor threw any kind of runtime exception, the stream from the connection would not be closed.

Make use of the Closeable interface to reduce redundancy.

Here is how I would write your code:

URLConnection connection = new URL("www.example.com").openConnection();
InputStream in = connection.getInputStream();
Closeable resource = in;
try {
  InputStreamReader isr = new InputStreamReader(in);
  resource = isr;
  BufferedReader br = new BufferedReader(isr);
  resource = br;
  String response = br.readLine();
} finally {
  resource.close();
}

Note that:

• no matter what kind of exception is thrown (runtime or checked) or where, the code does not leak stream resources
• there is no catch block; exceptions should be passed up to where the code can make a sensible decision about error handling; if this method was the right place, you'd surround all of the above with try/catch

A while back, I spent some time thinking about how to avoid leaking resources/data when things go wrong.

I think it will be better to place the closing methods in the finally block

Yes, always. Because an exception might occur and resources aren't released/closed properly.

You only need to close the most outer reader because it will be responsible for closing any enclosing readers.

Yes, it's ugly... for now. I think there are plans for an automatic resource management in Java.

I'd use apache commons IO for this, as others have suggested, mainly IOUtils.toString(InputStream) and IOUtils.closeQuietly(InputStream):

public String readFromUrl(final String url) {

    InputStream stream = null; // keep this for finally block

    try {
        stream = new URL(url).openConnection().getInputStream();  // don't keep unused locals
        return IOUtils.toString(stream);
    } catch (final IOException e) {
        // handle IO errors here (probably not like this)
        throw new IllegalStateException("Can't read URL " + url, e);
    } finally {
        // close the stream here, if it's null, it will be ignored
        IOUtils.closeQuietly(stream);
    }

}

You don't need multiple close statements for any of the nested streams and readers in java.io. It's very rare to need to close more than one thing in a single finally - most of the constructors can throw an exception, so you would be trying to close things you haven't created yet.

If you want to close the stream whether or not the read succeeds, then you need to put in into a finally.

Don't assign null to variables and then compare them to see whether something happened earlier; instead structure your program so the path where you close the stream can only be reached if the exception is not thrown. Apart from the variables used to iterate in for loops, variables should not need to change value - I tend to mark everything final unless there is a requirement to do otherwise. Having flags around your program to tell you how you got to the code currently being executed, and then changing behaviour based on those flags, is very much a procedural (not even structured) style of programming.

How you nest the try/catch/finally blocks depends on whether you want to handle the exceptions thrown by the different stages differently.

private static final String questionUrl = "http://stackoverflow.com/questions/3044510/";

public static void main ( String...args )
{
    try {
        final URLConnection connection = new URL ( args.length > 0 ? args[0] : questionUrl ).openConnection();

        final BufferedReader br = new BufferedReader ( new InputStreamReader (
                    connection.getInputStream(), getEncoding ( connection ) ) );

        try {
            final String response = br.readLine();

            System.out.println ( response );
        } catch ( IOException e ) {
            // exception handling for reading from reader
        } finally {
            // br is final and cannot be null. no need to check
            br.close();
        }
    } catch ( UnsupportedEncodingException  uee ) {
        // exception handling for unsupported character encoding
    } catch ( IOException e ) {
        // exception handling for connecting and opening reader
        // or for closing reader
    }
}

getEncoding needs to inspect the results of the connection's getContentEncoding() and getContentType() to determine the encoding of the web page; your code just uses the platform's default encoding, which may well be wrong.

Your example though is unusual in structured terms, since it is very procedural; normally you would separate the printing and the retrieving in a larger system, and allow the client code to handle any exception (or sometimes catch and create a custom exception):

public static void main ( String...args )
{
    final GetOneLine getOneLine = new GetOneLine();

    try {
        final String value = getOneLine.retrieve ( new URL ( args.length > 0 ? args[0] : questionUrl ) );
        System.out.println ( value );
    } catch ( IOException e ) {
        // exception handling for retrieving one line of text
    }
}

public String retrieve ( URL url ) throws IOException
{
    final URLConnection connection = url.openConnection();
    final InputStream in = connection.getInputStream();

    try {
        final BufferedReader br = new BufferedReader ( new InputStreamReader (
                    in, getEncoding ( connection ) ) );

        try {
            return br.readLine();
        } finally {
            br.close();
        }
    } finally {
        in.close();
    }
}

As McDowell pointed out, you may need to close the input stream if new InputStreamReader throws.

In scope of Java 8 I would use alike:

try(Resource resource = acquire()) {
    use(resource);
    reuse(resource);
}