You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ant.apache.org by Kev Jackson <ke...@it.fts-vn.com> on 2004/12/17 03:48:24 UTC
Re: Purpose of FileUtils.close(...)
>
> So what exactly *is* the intention of this method? IMHO, if an
> IOException was thrown when some stream was closed, it was probably
> for a reason, and probably something is broken, and probably the user
> should find out about it - and fix it - rather than have the problem
> be permanently suppressed from view. Maybe an exception closing a
> stream is harmless, but Ant surely doesn't know this; it could mean
> the last few Kb did not get flushed to disk, for example. E.g. I often
> see in Ant code like this (roughly adopted from
> ChangeLogTask.writeChangelog):
>
1.
My intention was only to uniform the codebase around the helper methods
that were presumably created for this sort of task. If you originally
swallow the exception (especially IOException - which I agree should be
reported somewhere, but currently aren't), but with a finally block that
first tests for null and then tries to close() stream/reader - and this
can be replaced by one line FileUtils.close(), then surely this is more
readable?
> OutputStream os = null;
> try {
> os = new FileOutputStream(...);
> printStuffToStream(os);
> } catch (IOException e) {
> throw new BuildException(e);
> } finally {
> FileUtils.close(os);
> }
>
2.
This follows the standard that I was aware of
1 try to do something
2 catch any exceptions and handle them gracefully
3 finally ensure that the program is in a state where we can continue or
shutdown without breaking anything else
> To my mind this is both harder to follow the flow of, and less robust
> than, the straightforward version:
>
> try {
> OutputStream os = new FileOutputStream(...);
> try {
> printStuffToStream(os);
> } finally {
> os.close();
> }
> } catch (IOException e) {
> throw new BuildException(e);
> }
>
3.
when does finally get called? After the inner try, or at the end of the
outer try block? Reading the spec only says that the VM honours the
fact that finally will always be called before returning back up the
call tree. Certain VM's could handle this differently (although I'm
pretty sure they'd all just execute the inner stuff first).
IMHO this structure is less clear than the one above.
> Here it is obvious than any IOException's get reported, and that an
> OutputStream will always be closed if it ever got created. You don't
> need any special utility method, or any dummy initial null value. You
> can even make the local var final if that floats your boat.
>
4.
The utility method is designed to swallow the exception and not report
it. So the rational of these changes is that you care about catching
IOExceptions in the general body of the code (FileNotFound etc etc), but
you don't want to report an exception when you're trying to clean up in
the finally method, as any exceptions would have already been reported
and indeed the stream etc would probably have been closed anyway, the
finally is simply used as a sanity check.
for example
private Class findClassInComponents(String name)
throws ClassNotFoundException {
// we need to search the components of the path to see if
// we can find the class we want.
InputStream stream = null;
String classFilename = getClassFilename(name);
try {
Enumeration e = pathComponents.elements();
while (e.hasMoreElements()) {
File pathComponent = (File) e.nextElement();
try {
stream = getResourceStream(pathComponent,
classFilename);
if (stream != null) {
log("Loaded from " + pathComponent + " "
+ classFilename, Project.MSG_DEBUG);
return getClassFromStream(stream, name,
pathComponent);
}
} catch (SecurityException se) {
throw se;
} catch (IOException ioe) {
// ioe.printStackTrace();
log("Exception reading component " + pathComponent
+ " (reason: " + ioe.getMessage() + ")",
Project.MSG_VERBOSE);
}
}
throw new ClassNotFoundException(name);
} finally {
try {
if (stream != null) {
stream.close();
}
} catch (IOException e) {
//ignore
}
}
}
Here the logging of any exceptions happens as normal and then in the
finally we just want to make sure that the stream is closed.
Unfortunately as the close method throws IOException, we have to have
yet another try/catch when we don't care about the exception at all.
FileUtils simply provides a tidier way of acheiveing this.
Is it a good thing to simply swallow exceptions? That's a whole other
story and I can see both sides of the argument but I'll sit on the fence ;).
> When there is more than one thing to be closed - e.g. an InputStream
> and an OutputStream - using nested try-finally blocks makes it clearer
> that everything gets closed, with no "if (os != null) {....}" weirdness:
>
> try {
> InputStream is = new FileInputStream(...);
> try {
> OutputStream os = new FileOutputStream(...);
> try {
> copyStuff(is, os);
> } finally {
> os.close();
> }
> } finally {
> is.close();
> }
> } catch (IOException e) {
> throw new BuildException(e);
> }
>
> Thoughts?
>
Finally...
I can see where your'e going with this. You use the catch to wrap
everything, and don't catch locally. The only problem is where you want
to log the exact message at the time of the exception do you lose the
ability when you catch at the end? Also I like the way you don't check
for nulls - is this NullPointerException safe? if 'is' is null when you
call close in finally (guaranteed to happen), you'll get a
NullPointerException - which is a RuntimeException, so will propogate
back to the user.
my thoughts...;)
Kev
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org
Re: Purpose of FileUtils.close(...)
Posted by Jesse Glick <je...@sun.com>.
Kev Jackson wrote:
>> So what exactly *is* the intention of this method? [...]
>
> 1.
> My intention was only to uniform the codebase around the helper methods
> that were presumably created for this sort of task.
Yes, I was really addressing other committers generally, sorry for the
confusion.
I should add that I don't think this issue is especially important
relative to other things, it just caught my eye and I wondered if others
cared.
>> try {
>> OutputStream os = new FileOutputStream(...);
>> try {
>> printStuffToStream(os);
>> } finally {
>> os.close();
>> }
>> } catch (IOException e) {
>> throw new BuildException(e);
>> }
>>
> 3.
> when does finally get called? After the inner try, or at the end of the
> outer try block?
After the inner try, of course - just considering
OutputStream os = new FileOutputStream(...);
try {
printStuffToStream(os);
} finally {
os.close();
}
in isolation, it is clear that 'os' is always closed.
> Reading the spec only says that the VM honours the
> fact that finally will always be called before returning back up the
> call tree. Certain VM's could handle this differently (although I'm
> pretty sure they'd all just execute the inner stuff first).
http://java.sun.com/docs/books/jls/second_edition/html/statements.doc.html#236653
"If execution of the try block completes abruptly because of a throw of
a value V [....] If the run-time type of V is not assignable to the
parameter of any catch clause of the try statement [which must be true
since there are no catch clauses in the inner try], then the finally
block is executed. Then there is a choice:
* If the finally block completes normally, then the try statement
completes abruptly because of a throw of the value V.
* If the finally block completes abruptly for reason S, then the try
statement completes abruptly for reason S (and the throw of value V is
discarded and forgotten)."
Note that an exception in finally "shadows" an exception in the main
block, but I doubt it matters much - probably you want to resolve *all*
exceptions so you could start with any of them.
> 4.
> The utility method is designed to swallow the exception and not report
> it.
I know, that's the problem.
> So the rational[e] of these changes is that you care about catching
> IOExceptions in the general body of the code (FileNotFound etc etc), but
> you don't want to report an exception when you're trying to clean up in
> the finally method, as any exceptions would have already been reported
> and indeed the stream etc would probably have been closed anyway, the
> finally is simply used as a sanity check.
My point is that if .close() throws an IOException, even if the main
body ran OK, something more serious could be wrong. Perhaps a final
flush to disk failed, for example. The current Ant code is basically
assuming that .close() doesn't do anything interesting. Maybe it doesn't
in practice, I don't know, but wouldn't it be safer to report such
problems? (And write code to selectively recover from them with only a
warning, if we can be sure that the user's requested action has in fact
completed.)
I guess .close() on InputStream is less relevant than on OutputStream.
> private Class findClassInComponents(String name)
> throws ClassNotFoundException {
> [...]
> InputStream stream = null;
> [...]
> try {
> [...]
> } finally {
> try {
> if (stream != null) {
> stream.close();
> }
> } catch (IOException e) {
> //ignore
> }
> }
> }
>
> Here the logging of any exceptions happens as normal and then in the
> finally we just want to make sure that the stream is closed.
> Unfortunately as the close method throws IOException, we have to have
> yet another try/catch when we don't care about the exception at all.
But if you wrap the entire thing in one try-catch for IOException, there
is not really any extra code needed.
> Is it a good thing to simply swallow exceptions? [...]
Well it is often harmless, and of course if you know for sure why the
exception might be thrown and that it is not a truly exceptional
situation, then fine. But once in a while some rare exception - "write
failed, disk full" - is critical to understanding why a whole program
failed to work quite right. If you get sources for the code that is
swallowing the exception, your best bet is to patch it to use
printStackTrace(), and that is really a pain. I have wasted hours trying
to figure out why a user of my code was occasionally seeing
ClassLoader.getResourceAsStream(...) return null in an odd place, until
I put in some heavy debugging code and found that getResource(...) was
fine but opening the stream failed with a stupid problem that was
swallowed but easily corrected once seen.
> I can see where your'e going with this. You use the catch to wrap
> everything, and don't catch locally. The only problem is where you want
> to log the exact message at the time of the exception do you lose the
> ability when you catch at the end?
Exception.getMessage() you mean? No, that's fine. Of course, there are
cases you where you really want to catch some exceptions early, log
them, and try to continue with something else... but it's not so common.
> Also I like the way you don't check
> for nulls - is this NullPointerException safe? if 'is' is null when you
> call close in finally (guaranteed to happen), [...]
InputStream is = new FileInputStream(...);
try {
[any code]
} finally {
is.close();
}
'is' cannot be null, unless of course you set it to null in the try block.
Also note that there is no need to wrap the FileInputStream block in
'finally': either it succeeds, in which case you continue to the 'try'
block, or it fails with an exception, in which case there is nothing to
close. You can't put anything between the constructor and the try block,
e.g.
Reader r = new WhateverReader(new FileInputStream(...));
try {
[any code]
} finally {
r.close();
}
is not safe - use
InputStream is = new FileInputStream(...);
try {
Reader r = new WhateverReader(is);
[any code]
} finally {
is.close();
}
-J.
--
Jesse Glick <ma...@sun.com> x22801
NetBeans, Open APIs <http://www.netbeans.org/>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@ant.apache.org
For additional commands, e-mail: dev-help@ant.apache.org