Elegant closure of InputStreams under exceptions?

[bugbear]

Even with Java's garbage collection, there is often
a new to close (or otherwise release) expensive
resource.

Classic instances include window and file handles.

This is easily handled using Java's try-catch-finally
model.

HandleThing h = new HandleThing(1,2,3);
try {

 

[Thomas Fritsch]

Even with Java's garbage collection, there is often
a new to close (or otherwise release) expensive
resource.

Classic instances include window and file handles.

This is easily handled using Java's try-catch-finally
model.

HandleThing h = new HandleThing(1,2,3);
try {
.
use h
.
.
} finally {
h.close();
}

These blocks can be nested to any depth,
and work well.

But the FilterInputStream class (and
any class that extends it) makes this model
unusable.

The problem is caused by 2 things.
1) a FilterInputStream closes its sub-stream
when it is itself closed.

2) Many InputStream don't like (i.e. throw exceptions)
if they are closed more than once.

So...

If a FilterInputStream is closed in an inner block,
it will ALSO close the InputStream that was opened
in an outer block.

So when the outer block's finally block does
its h.close(), it errors.

YUCK!

Am I missing trick here, or is it all as
bad as I think?

I would welcome any suggestions for elegant
implementation of correct closing under both
normal and exception cases.

BugBear

Instead of
finally {
h.close();
}
you could do
finally {
try {
h.close();
} catch(IOException e) {
}
}
It is not as elegant you as you want, but at least it works.

 

[bugbear]

bugbear schrieb:

Instead of
finally {
h.close();
}
you could do
finally {
try {
h.close();
} catch(IOException e) {
}
}
It is not as elegant you as you want, but at least it works.

It causes an Exception to be thrown in the normal
(i.e. non Exception) code path, which has efficiency
implications.

My best inelegant solution is:

h = new ...
try {
f = new SubClassOfFilterInputStream(h);
try {
... play with f ...
} finally {
f.close();
}
} catch (WhateverTheInnerCodeThrowsException wticte) {
h.close();
throw wticte;
}

But if the inner code can throw many different Exception this gets verbose,
and if the inner code throws an RTE, I miss it.

BugBear

 

[Robert Klemme]

It causes an Exception to be thrown in the normal
(i.e. non Exception) code path, which has efficiency
implications.

My best inelegant solution is:

h = new ...
try {
f = new SubClassOfFilterInputStream(h);
try {
... play with f ...
} finally {
f.close();
}
} catch (WhateverTheInnerCodeThrowsException wticte) {
h.close();
throw wticte;
}

But if the inner code can throw many different Exception this gets
verbose, and if the inner code throws an RTE, I miss it.

And you loose the original stack trace which might be needed sometimes.

We had a similar discussion recently (thread "")

I suggested this (note BufferedReader's constructor throws only for
illegal buffer sizes)

public List readEntireFile( String filename ) throws
IOException {
BufferedReader br = new BufferedReader(
new FileReader( filename ) );

try {
String line;
LinkedList sb=new LinkedList();
while ( (line=br.readline()) != null ) {
sb.add( line );
}
return sb;
}
finally {
try { br.close(); } catch ( IOException ignore ) {}
}
}

Thomas suggested

Some improvements:

// Note we are using the default character encoding,
// which is probably wrong.
public static List<String> readFileFully(
String filename
) throws IOException {
Reader rawReader = new FileReader(filename);
try {
BufferedReader reader = new BufferedReader(
rawReader
}
List<String> lines = new ArrayList<String>();
for (; {
String line = reader.readLine();
if (line == null) {
return lines;
}
lines.add(line);
}
} finally {
rawReader.close();
}
}

IMHO both approaches have their pros and cons. For example, my approach
does not loos the original exception while with the second example an
exception raised by close will shadow an exception raised in the block.

Kind regards

robert

 

[Thomas Hawtin]

I suggested this (note BufferedReader's constructor throws only for
illegal buffer sizes)

Not true. It can also throw due to out of memory of stack space (or some
fule has called Thread.stop).

public List readEntireFile( String filename ) throws
IOException {
BufferedReader br = new BufferedReader(
new FileReader( filename ) );

So this can still drop the stream.

The idiom is always: acquire-try-use-finally-dispose.

try {
String line;
LinkedList sb=new LinkedList();
while ( (line=br.readline()) != null ) {
sb.add( line );
}
return sb;
}
finally {
try { br.close(); } catch ( IOException ignore ) {}

This is just throwing a perfectly valid exception away, even if it is
the first. In this case closing an input stream that you have read
through probably isn't an issue, but in general very bad.

}
}

Thomas suggested

Some improvements:

// Note we are using the default character encoding,
// which is probably wrong.
public static List<String> readFileFully(
String filename
) throws IOException {
Reader rawReader = new FileReader(filename);
try {
BufferedReader reader = new BufferedReader(
rawReader
}
List<String> lines = new ArrayList<String>();
for (; {
String line = reader.readLine();
if (line == null) {
return lines;
}
lines.add(line);
}
} finally {
rawReader.close();
}
}

IMHO both approaches have their pros and cons. For example, my approach
does not loos the original exception while with the second example an
exception raised by close will shadow an exception raised in the block.

If the exception throw from the block was from the stream then any
exception from the close should indicate the problem exactly the same.
If there was some other exception, then that doesn't matter as we have a
serious problem with the resource.

Just remember to flush buffered output streams, at least in the
unexceptional case.

Tom Hawtin

 

[Eric Sosman]

Even with Java's garbage collection, there is often
a new to close (or otherwise release) expensive
resource.

Classic instances include window and file handles.

This is easily handled using Java's try-catch-finally
model.

HandleThing h = new HandleThing(1,2,3);
try {
.
use h
.
.
} finally {
h.close();
}

These blocks can be nested to any depth,
and work well.

But the FilterInputStream class (and
any class that extends it) makes this model
unusable.

The problem is caused by 2 things.
1) a FilterInputStream closes its sub-stream
when it is itself closed.

2) Many InputStream don't like (i.e. throw exceptions)
if they are closed more than once.

So...

If a FilterInputStream is closed in an inner block,
it will ALSO close the InputStream that was opened
in an outer block.

So when the outer block's finally block does
its h.close(), it errors.

YUCK!

Am I missing trick here, or is it all as
bad as I think?

I would welcome any suggestions for elegant
implementation of correct closing under both
normal and exception cases.

The solution seems to be "Once you've wrapped
something, don't unwrap it." That is, once you've
set up the IS and wrapped it in the FIS, you should
only manipulate the FIS thereafter.

This can become a little bit ugly at the point
where you're setting up the original wrappings, because
you've got to recover properly if (for example) you get
an exception from the IS before you've wrapped it in
the FIS. Here's how I did a similar thing in one of
my early efforts; it still looks like a reasonable
approach to me, but if someone has a better I'd not
be sorry to learn of it:

File file = ...;
InputStream stream = null;
BufferedReader reader = null;
try {
stream = new FileInputStream(file);
if (file.getName().endsWith(".gz"))
stream = new GZIPInputStream(stream);
reader = new BufferedReader(
new InputStreamReader(stream));
doThingsWithButDoNotClose(reader);
}
catch (...) { ... }
finally {
try {
if (reader != null)
reader.close();
else if (stream != null)
stream.close();
}
catch (IOException ex) { ... }
}

Here, the "nullity" of `reader' and/or `stream' tells me
which (if either) of them to close in the finally block.
The important point is that if I succeed in wrapping one
object in another, I never again touch the "wrapee."

 

[Thomas Hawtin]

The solution seems to be "Once you've wrapped
something, don't unwrap it." That is, once you've
set up the IS and wrapped it in the FIS, you should
only manipulate the FIS thereafter.

I don't think there's any reason to get religious about chronological
access.

Even your code can access as successfully wrapped stream.

This can become a little bit ugly at the point
where you're setting up the original wrappings, because
you've got to recover properly if (for example) you get
an exception from the IS before you've wrapped it in
the FIS. Here's how I did a similar thing in one of
my early efforts; it still looks like a reasonable
approach to me, but if someone has a better I'd not
be sorry to learn of it:

If I was going to do something like that I'd write:

File file = ...;
BufferedReader reader = null;
{
InputStream stream = null;
try {
stream = new FileInputStream(file);
if (file.getName().endsWith(".gz")) {
stream = new GZIPInputStream(stream);
}
reader = new BufferedReader(
new InputStreamReader(stream)
);
} finally {
if (reader == null) {
stream.close();
}
}
}
try {
doThingsWithButDoNotClose(reader);
} finally {
reader.close();
}

In fact, I'd probably put the construction is a separate method. With
the wrapping all bundled up in a method, then it becomes sensible not to
try and give access to the underlying resource.

Tom Hawtin

 

[Raymond DeCampo]

Even with Java's garbage collection, there is often
a new to close (or otherwise release) expensive
resource.

Classic instances include window and file handles.

This is easily handled using Java's try-catch-finally
model.

HandleThing h = new HandleThing(1,2,3);
try {
.
use h
.
.
} finally {
h.close();
}

These blocks can be nested to any depth,
and work well.

But the FilterInputStream class (and
any class that extends it) makes this model
unusable.

The problem is caused by 2 things.
1) a FilterInputStream closes its sub-stream
when it is itself closed.

2) Many InputStream don't like (i.e. throw exceptions)
if they are closed more than once.

According to the documentation, both InputStream, OutputStream, Reader
and Writer all implement Closeable. Closeable.close() clearly
indicates that calling close() on a stream that is already closed should
have no effect.

So...

If a FilterInputStream is closed in an inner block,
it will ALSO close the InputStream that was opened
in an outer block.

So when the outer block's finally block does
its h.close(), it errors.

If HandleThing implements Closeable, then this is a bug.

YUCK!

Am I missing trick here, or is it all as
bad as I think?

I would welcome any suggestions for elegant
implementation of correct closing under both
normal and exception cases.

BugBear

HTH,
Ray

 

[Thomas Hawtin]

According to the documentation, both InputStream, OutputStream, Reader
and Writer all implement Closeable. Closeable.close() clearly
indicates that calling close() on a stream that is already closed should
have no effect.

Some implementations are, er, negative variations. Even within java.io,
IIRC.

Tom Hawtin

 

[Raymond DeCampo]

Some implementations are, er, negative variations. Even within java.io,
IIRC.

Very possible (especially since Closeable was added after the fact,
IIRC). If you are aware of any, I would encourage you to report them as
bugs. If you are reluctant to report the bug yourself, let me know and
I will do it.

Of course in real world code it is sometimes necessary to code
work-arounds for bugs. This should be done by exception however, not as
a rule. (That is, until an implementation of Closeable barfs on the
second close(), do not code around it.)

Ray

 

[bugbear]

According to the documentation, both InputStream, OutputStream, Reader
and Writer all implement Closeable. Closeable.close() clearly
indicates that calling close() on a stream that is already closed should
have no effect.

OK. I think one of the InputStream subclasses I'm using
has a bug...

My eventual solution, which may not be universally applicable,
was to use this sub-class of InputStreamFilter (!)

public class CloseOnceInputStream extends FilterInputStream {
private boolean open = true;
public CloseOnceInputStream(InputStream is) {
super(is);
}
/**
closes the filtered InputStream, if it's open
*/
public void close() throws IOException {
if(open) {
super.close();
open = false;
}
}
}

It appears that this is an after-the-fact external bug fix for
some broken code.

But it allows my originally posted "dream code" to function
as intended.

Thanks to all for the constructive advice and information.

BugBear

 

[bugbear]

Very possible (especially since Closeable was added after the fact,
IIRC).

Yep - new for 1.5

BugBear