Closing Files that Weren't Successfully Opened

[KevinSimonson]

Almost from as far back in my software education as I can remember,
people who would know have told me that it's good practice to close
one's files when one is done with them, so when I write programs that
open files I always try to do that. But I think I've found some grey
area that makes this policy a little ambiguous.

Whenever I write a program that uses the classes <File> and <Scanner>
to read in lines of text from a file, when I go to compile it my
compiler declares, "unreported exception
java.io.FileNotFoundException; must be caught or declared to be
thrown". And whenever I write a program that uses the classes
<FileWriter>, <BufferedWriter>, and <PrintWriter> to write lines of
text to a file, when I go to compile it my compiler gives a similar
declaration: "unreported exception java.io.IOException; must be caught
or declared to be thrown".

So, being pretty obedient to what the compiler tells me to do when I
can figure out what that is, I always add a <try-catch> block to
handle the mentioned exceptions.

My question is this. If I declare a variable <scnnr> to be of type
<Scanner>, and try to open it with the statement <scnnr = new
Scanner( new File( "Xy.Txt"))>, and a <FileNotFoundException> gets
thrown, should I still do a <scnnr.close()>, say in the <catch>
block? And similarly, if I declare <prntWrtr> to be of type
<PrintWriter> and try to open it with the statement <prntWrtr = new
PrintWriter( new BufferedWriter( new FileWriter( "Xy.Txt")))>, and an
<IOException> gets thown, should I still do a <prntWrtr.close()>, also
probably in the <catch> block? Or in each such situation can I
conclude that since an exception occurred while I was attempting to
open the respective file, the variables will still each both be
<null>, and therefore I don't have to do anything?

Kevin Simonson

 

[Stanimir Stamenkov]

Mon, 14 Mar 2011 15:27:26 -0700 (PDT), /KevinSimonson/:

My question is this. If I declare a variable <scnnr> to be of type
<Scanner>, and try to open it with the statement <scnnr = new
Scanner( new File( "Xy.Txt"))>, and a <FileNotFoundException> gets
thrown, should I still do a <scnnr.close()>, say in the <catch>
block? And similarly, if I declare <prntWrtr> to be of type
<PrintWriter> and try to open it with the statement <prntWrtr =
new PrintWriter( new BufferedWriter( new FileWriter( "Xy.Txt")))>,
and an <IOException> gets thown, should I still do a
<prntWrtr.close()>, also probably in the <catch> block? Or in each
such situation can I conclude that since an exception occurred
while I was attempting to open the respective file, the variables
will still each both be <null>, and therefore I don't have to do
anything?

As you've noticed, if the object constructor throws, you'll get no
object reference to invoke a method on. It is responsibility of the
construction code to clean any unreachable resources allocated prior
throwing the exception.

 

[Andreas Leitgeb]

Mon, 14 Mar 2011 15:27:26 -0700 (PDT), /KevinSimonson/:
As you've noticed, if the object constructor throws, you'll get no
object reference to invoke a method on. It is responsibility of the
construction code to clean any unreachable resources allocated prior
throwing the exception.

This responsibility covers only stuff that has been allocated within
that constructor that throws. If Scanner were to throw, then it would
not clean up the open File passed to it.
To solve it correctly, you(the OP) do your construction work stepwise:

File file; Scanner scnnr;
try {
file = new File ("Xy.Txt");
scnnr= new Scanner ( file );
} catch (FileNotFoundException e) {
if (file != null) { file.close(); }
... other cleanup
} catch (SomeOtherException e) {
// ditto
} ...

Yes, that isn't as "beautiful" as putting several "new"s into a single
line, but as you saw, that beauty came with a price (possible ressource-
leakage), that you seem not to like (and correctly so).

 

[Andreas Leitgeb]

This responsibility covers only stuff that has been allocated within
that constructor that throws. If Scanner were to throw, then it would
not clean up the open File passed to it.
To solve it correctly, you(the OP) do your construction work stepwise:
File file; Scanner scnnr;
try {
file = new File ("Xy.Txt");
scnnr= new Scanner ( file );
} catch (FileNotFoundException e) {
if (file != null) { file.close(); }
... other cleanup
} catch (SomeOtherException e) {
// ditto
} ...
Yes, that isn't as "beautiful" as putting several "new"s into a single
line, but as you saw, that beauty came with a price (possible ressource-
leakage), that you seem not to like (and correctly so).

On re-read it seems I have to clarify a bit: That thing with separate
construction seems not all that relevant to the File+Scanner combo.

It's more of a general thing, and would principially apply on any chain
of constructions, where any inner step really allocates resources, that
ask for explicit release (which File doesn't).
Maybe it applies better to the Writer-chain, but I'm now too lazy to look
that up for documented special cases of convenience-cleanup.

 

[Dagon]

it's good practice to close one's files when one is done with them
True.

So, being pretty obedient to what the compiler tells me to do when I
can figure out what that is, I always add a <try-catch> block to
handle the mentioned exceptions.

You need to apply some amount of thought in addition to obedience. The
compiler says you must catch _or_ declare the exception. Adding a try-catch
block without knowing why is not advised.

My question is this. If I declare a variable <scnnr> to be of type
<Scanner>, and try to open it with the statement <scnnr = new
Scanner( new File( "Xy.Txt"))>, and a <FileNotFoundException> gets
thrown, should I still do a <scnnr.close()>, say in the <catch>
block?

No. scnnr has not been initialized - an exception was thrown in trying to
instantiate a Scanner to assign to scnnr. scnnr.close() will throw
NullPointerException.

open it with the statement <prntWrtr = new PrintWriter(
new BufferedWriter( new FileWriter( "Xy.Txt")))>, and an
<IOException> gets thown, should I still do a <prntWrtr.close()>, also
probably in the <catch> block?

Same answer. There is no PrintWriter instance for you to close.

Or in each such situation can I
conclude that since an exception occurred while I was attempting to
open the respective file, the variables will still each both be
<null>, and therefore I don't have to do anything?

Correct. However, a little bit of care must be taken if you have multiple
statements in the try/catch block. If the creation of the object succeeds,
but a later usage throws an exception, you should close it.

It's not uncommon to see code like:
finally {
if (someResource != null) {
try {
someResource.close();
} catch (exceptionsCloseCanThrow ecct) {
logger.error("WTF!", ecct);
}
}
}

This is ugly, and an admission that you haven't tracked the state of your
resource very well, but it does always make sure you've closed the resource if
possible.

 

[Lew]

It's not uncommon to see code like:
finally {
if (someResource != null) {
try {
someResource.close();
} catch (exceptionsCloseCanThrow ecct) {
logger.error("WTF!", ecct);
}
}
}

This is ugly, and an admission that you haven't tracked the state of your
resource very well, but it does always make sure you've closed the resource if
possible.

Until the new resource-release idioms come on line, there's a certain ugliness
that's unavoidable.

I like the assign-once idioms, i.e., no initial assignment to 'null', and
therefore assertable non-nullity.

Something like:

public void useResource( String src, String dst )
{
final BufferedReader reader;
try
{
reader = new BufferedReader( new FileReader( src ));
}
catch ( IOException exc )
{
final String msg = "cannot open \"+ src +"\". "
+ exc.getLocalizedMessage();
logger.error( msg, exc );
throw new IllegalStateException( msg, exc );
}
assert reader != null;

final BufferedWriter writer;
try
{
writer = new BufferedWriter( new FileWriter( dst ));
}
catch ( IOException exc )
{
final String msg = "cannot open \"+ dst +"\". "
+ exc.getLocalizedMessage();
logger.error( msg, exc );
try
{
reader.close();
}
catch ( IOException cex )
{
final String cmsg = "cannot close \"+ src +"\". "
+ cex.getLocalizedMessage();
logger.error( cmsg, cex );
}
throw new IllegalStateException( msg, exc );
}
assert writer != null;

try
{
workWith( reader, writer ); // no closing inside that method
}
finally
{
try
{
reader.close();
}
catch ( IOException cex )
{
final String cmsg = "cannot close \"+ src +"\". "
+ cex.getLocalizedMessage();
logger.error( cmsg, cex );
}
try
{
writer.close();
}
catch ( IOException cex )
{
final String cmsg = "cannot close \"+ dst +"\". "
+ cex.getLocalizedMessage();
logger.error( cmsg, cex );
}
}
}

 

[Lawrence D'Oliveiro]

It's not uncommon to see code like:
finally {
if (someResource != null) {
try {
someResource.close();
} catch (exceptionsCloseCanThrow ecct) {
logger.error("WTF!", ecct);
}
}
}

This is ugly, and an admission that you haven't tracked the state of your
resource very well ...

What’s the alternative?

 

[Robert Klemme]

What’s the alternative?

The alternative is to create and assign outside the block and do the
close in finally - unconditionally:

final FileInputStream fin = new FileInputStream("foo.txt");
try {
final byte[] buffer = new byte[1024];
for (int read;(read = fin.read(buffer))>=0 {
// do whatever with buffer[0,read[
}
}
finally {
// no null or other check needed!
fin.close();
}

If you want to ensure an exception from close() does not shadow
another error you can do

static void close(Closable c) {
try {
c.close();
}
catch (IOException e) {
// ignore or log
}
}

and use that in the finally block.

If you want BufferedReader you can do

final BufferedReader reader =
new BufferedReader(new InputStreamReader(new
FileInputStream("foo.txt")));
try {
for (String line; (line = reader.readLine()) != null {
System.out.println("Found line: " + line);
}
}
finally {
reader.close();
}

I would declare IOException on the method that contains this code to
leave IO error handling to callers.

Kind regards

robert

 

[Michal Kleczek]

If you want BufferedReader you can do

final BufferedReader reader =
new BufferedReader(new InputStreamReader(new
FileInputStream("foo.txt")));
try {
for (String line; (line = reader.readLine()) != null {
System.out.println("Found line: " + line);
}
}
finally {
reader.close();
}

That only works because both InputStreamReader and BufferedReader
constructors don't throw.
But it is not safe to do the same for:
final ObjectInputStream ois =
new ObjectInputStream(new FileInputStream("foo.txt"));

Sometimes it would be good to have destructors...

 

[Lew]

That only works because both InputStreamReader and BufferedReader
constructors don't throw.
But it is not safe to do the same for:
final ObjectInputStream ois =
new ObjectInputStream(new FileInputStream("foo.txt"));

That's covered upthread five and a half hours prior to the cited post.

Sometimes it would be good to have destructors...

Nonsense. Destructors are to release memory. This is about external
resources. What Java needs are 'finally' blocks. Hey, good news! It's got
them! Yay! Problem solved!

 

[Michal Kleczek]

That's covered upthread five and a half hours prior to the cited post.

Maybe I'm missing it but could not find anything related.
If you're talking about your post - there is still this
new BufferedReader(new ...) thingy hanging around there
and nothing about watching out for leaked resources if both
chained constructors can throw.

Nonsense. Destructors are to release memory.

Who told you that?

This is about external
resources.

And that is also something that destructors can (and should) release.
Please - google "RAII".

What Java needs are 'finally' blocks. Hey, good news! It's
got them! Yay! Problem solved!

The point is they are not really straightforward to use
and cause a lot of clutter in the code.
This is not only my opinion: introduction of "with" statement in C#,
proposed changes to Java 7 and this thread itself speak for themselves.

 

[Robert Klemme]

That only works because both InputStreamReader and BufferedReader
constructors don't throw.
But it is not safe to do the same for:
final ObjectInputStream ois =
  new ObjectInputStream(new FileInputStream("foo.txt"));

You just need to apply the same pattern several times:

final new FileInputStream fin = FileInputStream("foo.txt");
try {
final ObjectInputStream ois = new ObjectInputStream(fin);
try {
...
}
finally {
ois.close();
}
}
finally {
// multiple close() do not hurt!
fin.close();
}

Or you create a separate method for opening

private static ObjectInputStream open(String fileName) throws
IOException {
final new FileInputStream fin = FileInputStream("foo.txt");
boolean ok = false;
try {
final ObjectInputStream oin = new ObjectInputStream(fin);
ok = true;
return oid;
}
finally {
if (!ok) fin.close();
}
}

Or, a bit shorter:

private static ObjectInputStream open(String fileName) throws
IOException {
final new FileInputStream fin = FileInputStream("foo.txt");
try {
return new ObjectInputStream(fin);
}
catch (IOException e) {
fin.close();
throw e;
}
}

Kind regards

robert

 

[Lew]

Maybe I'm missing it but could not find anything related.

It's my post from Mar 15, 1:17 am, New York time.

If you're talking about your post - there is still this

Well, duhhh.

new BufferedReader(new ...) thingy hanging around there
and nothing about watching out for leaked resources if both
chained constructors can throw.

Huh? Yes there is, too - it's called a 'catch' block. Notice that if
the constructors throw an exception in my example that it's caught and
all resources are guaranteed to be released, through the use of
'finally'.

You *did* notice that "this new BufferedReader(new ...) thingy hanging
around there" was inside a 'try...catch' structure, didn't you? How
is that "nothing about watching out for leaked resources"? You *did*
notice that the 'catch' blocks release resources if the whole shebang
doesn't allocate, didn't you? You *did* notice the 'finally' block
that guarantees (RAII-ishly!) release of resources at the end, didn't
you? If not, how could you possibly miss all that code in my post?
It was the largest part of it!

The whole freaking *point* of my post was to show one way to guarantee
release of resources!

Look, make all the legitimate points you want, but don't lie.

Who told you that?


And that is also something that destructors can (and should) release.

It's something that should be released, but not necessarily in a
destructor.

For proof, consider that Java doesn't have destructors yet you are
still perfectly capable of releasing resources in a guaranteed way
using 'finally'.

Please - google "RAII".

Which is what Java uses 'finally' for.

The point is they are not really straightforward to use
and cause a lot of clutter in the code.
This is not only my opinion: introduction of "with" statement in C#,
proposed changes to Java 7 and this thread itself speak for themselves.

Yes, those add a little sugar, but they don't add new capability.

I guess one man's "clutter" is another man's explicit logic.

See my post that you erroneously claimed had "nothing" to release
resources for an example of how to guarantee resource release in Java
using 'try...catch...finally'.

 

[Michal Kleczek]

[snip]
The whole freaking *point* of my post was to show one way to guarantee
release of resources!

Look, make all the legitimate points you want, but don't lie.

You're right - I've read your post again and I admit your way of handling
this does not have problems I wanted to raise. But I was not replying to
_your_ post but to the one cited at the top.
If you just wanted to point out that your way of managing resources
described in another branch of this thread is better - then point taken.

It's something that should be released, but not necessarily in a
destructor.

True. But:
1. Surely destructors are not only to release memory.
2. _Most of the times_ resources acquired during the lifetime of an
object should be released in the destructor.

For proof, consider that Java doesn't have destructors yet you are still
perfectly capable of releasing resources in a guaranteed way using
'finally'.

I didn't say anywhere that programs written in Java are not capable of
releasing resources in a guaranteed way.
What I said was:

1. This idiom:

final InputStreamWrapper wrapper =
new InputStreamWrapper(new ResourceAcquiringInputStream(resource));
try {
//process wrapper
}
finally {
wrapper.close();
}

while common is dangerous because it _may_ lead to resource leaks.

2. In this particular case (resource acquiring/releasing) I miss
destructors in Java since try/catch/finally construct makes code less
readable when I need to manage several resources.

 

[Lawrence D'Oliveiro]

Sometimes it would be good to have destructors...

You want a close/finalize call that will never return an error.

 

[Lawrence D'Oliveiro]

2. In this particular case (resource acquiring/releasing) I miss
destructors in Java since try/catch/finally construct makes code less
readable when I need to manage several resources.

I wrote about a scalable technique for dealing with this years ago
<http://www.geek-central.gen.nz/peeves/programming_discipline.html>.

 

[Roedy Green]

The key to understanding is this. If your open fails there will be no
Stream object created, and there will be no reference to it in your
handle. If you try to close it, you will get a NullPointerException.
If the logic lets you get to your close without a successful open do
this:

if ( handle != null ){ handle.close();}
--
Roedy Green Canadian Mind Products
http://mindprod.com
Refactor early. If you procrastinate, you will have
even more code to adjust based on the faulty design.
..

 

[Michal Kleczek]

Lew wrote:
Michal Kleczek wrote:
Robert Klemme wrote:

If you want BufferedReader you can do

final BufferedReader reader =
new BufferedReader(new InputStreamReader(new
FileInputStream("foo.txt")));
try {
for (String line; (line = reader.readLine()) != null {
System.out.println("Found line: " + line);
}
}
finally {
reader.close();
}

That only works because both InputStreamReader and BufferedReader
constructors don't throw.
But it is not safe to do the same for: final ObjectInputStream ois =
new ObjectInputStream(new FileInputStream("foo.txt"));

[snip]
The whole freaking *point* of my post was to show one way to guarantee
release of resources!

Look, make all the legitimate points you want, but don't lie.

You're right - I've read your post again and I admit your way of handling
this does not have problems I wanted to raise.

I've re-read your post again and I think my point holds. This code (copied
from your post):

public void useResource( String src, String dst )
{
final BufferedReader reader;
try
{
reader = new BufferedReader( new FileReader( src ));
}
catch ( IOException exc )
{
final String msg = "cannot open \"+ src +"\". "
+ exc.getLocalizedMessage();
logger.error( msg, exc );
throw new IllegalStateException( msg, exc );
}
assert reader != null;

is not exception safe - if BufferedReader constructor throws you've got a
resource leak.

[snip]
As you can see above the problem is not solved. Above code would be
perfectly valid (in terms of exception safety) if we had destructors (and -
of course - if FileReader destructor was implemented to release resources
propely).

 

[Lew]

You're right - I've read your post again and I admit your way of handling
this does not have problems I wanted to raise. But I was not replying to
_your_ post but to the one cited at the top.
If you just wanted to point out that your way of managing resources
described in another branch of this thread is better - then point taken.

Not better, just possible.

 

[Lew]

is not exception safe - if BufferedReader constructor throws you've got a
resource leak.

Hence the 'catch' block. What resource can leak there?