Closing decorated streams

[Philipp]

Hello, I use the following code but am not sure that it is the best or
correct way to close the stream. In particular, if url.openStream()
succeeds I have an open stream, but if any of the two other
constructors throws an exception, reader will be null and the stream
will remain open even after the finally block.

public class WhatToClose {
public static void main(String[] args) {
BufferedReader reader = null;
try{
URL url = new URL("http://www.yahoo.com/");
reader = new BufferedReader(new InputStreamReader(url.openStream
()));

// use reader here
String line = null;
while (( line = reader.readLine()) != null){
System.out.println(line);
}
} catch (Exception e) {
e.printStackTrace();
} finally {
if(reader != null){
try{
reader.close();
} catch (IOException e) {
e.printStackTrace();
}
}
}
}
}


I could allocate three refs (see below) buts that's really a PITA. How
do you do it?

public static void main(String[] args) {
InputStream is = null;
InputStreamReader isr = null;
BufferedReader br = null;
try{
URL url = new URL("http://www.yahoo.com/");
is = url.openStream();
isr = new InputStreamReader(is);
br = new BufferedReader(isr);
String line = null;
while (( line = br.readLine()) != null){
System.out.println(line);
}
} catch (Exception e) {
e.printStackTrace();
} finally {
if(br != null){
try{
br.close();
} catch (IOException e) {
e.printStackTrace();
}
}
if(isr != null){
try{
isr.close();
} catch (IOException e) {
e.printStackTrace();
}
}
if(is != null){
try{
is.close();
} catch (IOException e) {
e.printStackTrace();
}
}
}
}

Thanks Phil

 

[Knute Johnson]

Hello, I use the following code but am not sure that it is the best or
correct way to close the stream. In particular, if url.openStream()
succeeds I have an open stream, but if any of the two other
constructors throws an exception, reader will be null and the stream
will remain open even after the finally block.

public class WhatToClose {
public static void main(String[] args) {
BufferedReader reader = null;
try{
URL url = new URL("http://www.yahoo.com/");
reader = new BufferedReader(new InputStreamReader(url.openStream
()));

// use reader here
String line = null;
while (( line = reader.readLine()) != null){
System.out.println(line);
}
} catch (Exception e) {
e.printStackTrace();
} finally {
if(reader != null){
try{
reader.close();
} catch (IOException e) {
e.printStackTrace();
}
}
}
}
}


I could allocate three refs (see below) buts that's really a PITA. How
do you do it?

public static void main(String[] args) {
InputStream is = null;
InputStreamReader isr = null;
BufferedReader br = null;
try{
URL url = new URL("http://www.yahoo.com/");
is = url.openStream();
isr = new InputStreamReader(is);
br = new BufferedReader(isr);
String line = null;
while (( line = br.readLine()) != null){
System.out.println(line);
}
} catch (Exception e) {
e.printStackTrace();
} finally {
if(br != null){
try{
br.close();
} catch (IOException e) {
e.printStackTrace();
}
}
if(isr != null){
try{
isr.close();
} catch (IOException e) {
e.printStackTrace();
}
}
if(is != null){
try{
is.close();
} catch (IOException e) {
e.printStackTrace();
}
}
}
}

Thanks Phil

For normal streams, closing the top one is fine and will close the
others. I seem to recall some issue with URLConnection streams however.
I think there was a recent thread.

 

[Lew]

Hello, I use the following code but am not sure that it is the best or
correct way to close the stream. In particular, if url.openStream()
succeeds I have an open stream, but if any of the two other
constructors throws an exception, reader will be null and the stream
will remain open even after the finally block.

public class WhatToClose {
  public static void main(String[] args) {
    BufferedReader reader = null;

There's another idiom that skips the 'null' assignment - see below.

    try{
      URL url = new URL("http://www.yahoo.com/");
      reader = new BufferedReader(new InputStreamReader(url.openStream
()));

      // use reader here
      String line = null;

No need to superfluously assign a spurious 'null' to 'line'.

      while (( line = reader.readLine()) != null){
        System.out.println(line);
      }
    } catch (Exception e) {
      e.printStackTrace();
    } finally {
      if(reader != null){
        try{
          reader.close();

You could just trap and close the URL stream rather than the reader.

        } catch (IOException e) {
          e.printStackTrace();
        }
      }
    }
  }

}

I could allocate three refs (see below) buts that's really a PITA. How
do you do it?

public static void main(String[] args) {
  InputStream is = null;
  InputStreamReader isr = null;
  BufferedReader br = null;

This should not be necessary.

One way is to chain the I/O classes in separate try-catch blocks, so
that a later nullity check is unnecessary; you can use 'assert'
statements instead. The result is verbose but precise control over
what happens and how it's logged, and the safety of assertably
initialized streams and readers.

public static void main( String [] args )
{
final InputStream is;
try
{
is = url.openStream();
}
catch ( IOException exc )
{
logger.error( "Cannot open URL stream. " + exc.getMessage() );
return;
}
assert is != null;

final BufferedReader br;
try
{
br = new BufferedReader( new InputStreamReader( is ));
}
catch ( IOException exc )
{
logger.error( "Cannot open URL stream reader. " + exc.getMessage
() );
try
{
is.close();
}
catch ( IOException closex )
{
logger.error( "Cannot close URL stream. " + closex.getMessage
() );
}
return;
}
assert br != null;

try
{
for ( String line; (line = br.readLine()) != null; )
{
System.out.println(line);
}
}
catch ( IOException exc )
{
logger.error( "Cannot read URL stream reader. " + exc.getMessage
() );
}
finally
{
try
{
br.close();
}
catch ( IOException closex )
{
logger.error( "Cannot close URL stream reader. " +
closex.getMessage() );
}
}
}

 

[Knute Johnson]

Hello, I use the following code but am not sure that it is the best or
correct way to close the stream. In particular, if url.openStream()
succeeds I have an open stream, but if any of the two other
constructors throws an exception, reader will be null and the stream
will remain open even after the finally block.

public class WhatToClose {
public static void main(String[] args) {
BufferedReader reader = null;

There's another idiom that skips the 'null' assignment - see below.

try{
URL url = new URL("http://www.yahoo.com/");
reader = new BufferedReader(new InputStreamReader(url.openStream
()));

// use reader here
String line = null;

No need to superfluously assign a spurious 'null' to 'line'.

while (( line = reader.readLine()) != null){
System.out.println(line);
}
} catch (Exception e) {
e.printStackTrace();
} finally {
if(reader != null){
try{
reader.close();

You could just trap and close the URL stream rather than the reader.

} catch (IOException e) {
e.printStackTrace();
}
}
}
}

}

I could allocate three refs (see below) buts that's really a PITA. How
do you do it?

public static void main(String[] args) {
InputStream is = null;
InputStreamReader isr = null;
BufferedReader br = null;

This should not be necessary.

One way is to chain the I/O classes in separate try-catch blocks, so
that a later nullity check is unnecessary; you can use 'assert'
statements instead. The result is verbose but precise control over
what happens and how it's logged, and the safety of assertably
initialized streams and readers.

public static void main( String [] args )
{
final InputStream is;
try
{
is = url.openStream();
}
catch ( IOException exc )
{
logger.error( "Cannot open URL stream. " + exc.getMessage() );
return;
}
assert is != null;

final BufferedReader br;
try
{
br = new BufferedReader( new InputStreamReader( is ));
}
catch ( IOException exc )
{
logger.error( "Cannot open URL stream reader. " + exc.getMessage
() );
try
{
is.close();
}
catch ( IOException closex )
{
logger.error( "Cannot close URL stream. " + closex.getMessage
() );
}
return;
}
assert br != null;

try
{
for ( String line; (line = br.readLine()) != null; )
{
System.out.println(line);
}
}
catch ( IOException exc )
{
logger.error( "Cannot read URL stream reader. " + exc.getMessage
() );
}
finally
{
try
{
br.close();
}
catch ( IOException closex )
{
logger.error( "Cannot close URL stream reader. " +
closex.getMessage() );
}
}
}

Lew:

That's a pattern I've not seen before and I really like it, especially
the for loop for reading the lines. The return in the catch block is
pretty nifty too.

I am curious though if you really think that all those levels are really
necessary? The only place that can throw an I/O exception is the
URL.getInputStream() method and if it succeeds then the BufferedReader
constructor will succeed. So closing the BufferedReader will suffice.

 

[Tom Anderson]

Hello, I use the following code but am not sure that it is the best or
correct way to close the stream. In particular, if url.openStream()
succeeds I have an open stream, but if any of the two other
constructors throws an exception, reader will be null and the stream
will remain open even after the finally block.

In this case, i'd exploit the knowledge that the upper-layer streams are
lightweight, and don't really need to be closed - if they aren't closed
but are garbage collected, that's fine. So i'd do:

InputStream urlStream = url.openStream();
try {
BufferedReader reader = new BufferedReader(new InputStreamReader(urlStream));
// use reader
}
finally {
urlStream.close();
}

In the more general case, where you might have another heavyweight stream
that needs to be explicitly closed in the stack, i'd think about keeping a
pointer to the topmost stream, so i could just close that. Since that
stream could actually be a reader, i'd make the variable a Closeable. Like
so:

InputStream urlStream = url.openStream();
Closeable topStream = urlStream;
try {
Reader isr = new InputStreamReader(urlStream)'
topStream = isr;
BufferedReader reader = new BufferedReader(isr);
topStream = reader;
// use reader here
}
finally {
topStream.close();
}

Yes, you have to write out each construction as a separate statement, and
then add a line to update topStream. But at least you only have to do the
closing once. Note that you actually only have to do the separation and
updating for each heavyweight stream in the stack - lightweight streams
can be constructed inline. Like (assuming GZIPInputStream uses libgzip
with a native context object - i could be wrong about that):

InputStream urlStream = url.openStream();
Closeable topStream = urlStream;
try {
InputStream gz = new GZIPInputStream(new BufferedInputStream(urlStream));
topStream = gz;
BufferedReader reader = new BufferedReader(new InputStreamReader(gz));
// use reader here
}
finally {
topStream.close();
}

tom

 

[Lew]

One way is to chain the I/O classes in separate try-catch blocks, so
that a later nullity check is unnecessary; you can use 'assert'
statements instead. The result is verbose but precise control over
what happens and how it's logged, and the safety of assertably
initialized streams and readers.

public static void main( String [] args )
{
final InputStream is;
try
{
is = url.openStream();
}
catch ( IOException exc )
{
logger.error( "Cannot open URL stream. " + exc.getMessage() );
return;
}
assert is != null;

final BufferedReader br;
try
{
br = new BufferedReader( new InputStreamReader( is ));
}
catch ( IOException exc )
{
logger.error( "Cannot open URL stream reader. " + exc.getMessage
() );
try
{
is.close();
}
catch ( IOException closex )
{
logger.error( "Cannot close URL stream. " + closex.getMessage
() );
}
return;
}
assert br != null;

try
{
for ( String line; (line = br.readLine()) != null; )
{
System.out.println(line);
}
}
catch ( IOException exc )
{
logger.error( "Cannot read URL stream reader. " + exc.getMessage
() );
}
finally
{
try
{
br.close();
}
catch ( IOException closex )
{
logger.error( "Cannot close URL stream reader. " +
closex.getMessage() );
}
}
}

That's a pattern I've not seen before and I really like it, especially
the for loop for reading the lines. The return in the catch block is
pretty nifty too.

The return in the catch block is necessary to ensure the asserted invariant.

I am curious though if you really think that all those levels are really
necessary? The only place that can throw an I/O exception is the
URL.getInputStream() method and if it succeeds then the BufferedReader
constructor will succeed. So closing the BufferedReader will suffice.

I agree with you in this case. So the first try block would handle:

final BufferedReader br;
try
{
br = new BufferedReader( new InputStreamReader( url.openStream() ));
}
catch ( IOException exc )
{
logger.error( "Cannot open URL stream reader. " + exc.getMessage() );
return;
}
assert br != null;

The next try block would actually use 'br' and close it in its finally block.

The example I gave first was to address the OP's specific concern that the
reader assignments might fail separately from the stream retrieval, and to
illustrate the flexibility of the technique. Notice how easily the idiom
refactors to increase or decrease level of detail and granularity of control
by simply splitting out or merging try-catch blocks. The key to the idiom is
that one or more try-catch blocks establish an invariant of a valid resource
handle (the stream or reader instance), and a separate try-finally block (with
possible catch clauses) uses that guaranteed resource and disposes of it in
its finally section.

 

[Knute Johnson]

One way is to chain the I/O classes in separate try-catch blocks, so
that a later nullity check is unnecessary; you can use 'assert'
statements instead. The result is verbose but precise control over
what happens and how it's logged, and the safety of assertably
initialized streams and readers.

public static void main( String [] args )
{
final InputStream is;
try
{
is = url.openStream();
}
catch ( IOException exc )
{
logger.error( "Cannot open URL stream. " + exc.getMessage() );
return;
}
assert is != null;

final BufferedReader br;
try
{
br = new BufferedReader( new InputStreamReader( is ));
}
catch ( IOException exc )
{
logger.error( "Cannot open URL stream reader. " + exc.getMessage
() );
try
{
is.close();
}
catch ( IOException closex )
{
logger.error( "Cannot close URL stream. " + closex.getMessage
() );
}
return;
}
assert br != null;

try
{
for ( String line; (line = br.readLine()) != null; )
{
System.out.println(line);
}
}
catch ( IOException exc )
{
logger.error( "Cannot read URL stream reader. " + exc.getMessage
() );
}
finally
{
try
{
br.close();
}
catch ( IOException closex )
{
logger.error( "Cannot close URL stream reader. " +
closex.getMessage() );
}
}
}

That's a pattern I've not seen before and I really like it, especially
the for loop for reading the lines. The return in the catch block is
pretty nifty too.

The return in the catch block is necessary to ensure the asserted
invariant.

I am curious though if you really think that all those levels are
really necessary? The only place that can throw an I/O exception is
the URL.getInputStream() method and if it succeeds then the
BufferedReader constructor will succeed. So closing the
BufferedReader will suffice.

I agree with you in this case. So the first try block would handle:

final BufferedReader br;
try
{
br = new BufferedReader( new InputStreamReader( url.openStream() ));
}
catch ( IOException exc )
{
logger.error( "Cannot open URL stream reader. " + exc.getMessage() );
return;
}
assert br != null;

The next try block would actually use 'br' and close it in its finally
block.

The example I gave first was to address the OP's specific concern that
the reader assignments might fail separately from the stream retrieval,
and to illustrate the flexibility of the technique. Notice how easily
the idiom refactors to increase or decrease level of detail and
granularity of control by simply splitting out or merging try-catch
blocks. The key to the idiom is that one or more try-catch blocks
establish an invariant of a valid resource handle (the stream or reader
instance), and a separate try-finally block (with possible catch
clauses) uses that guaranteed resource and disposes of it in its finally
section.

Thanks Lew.