ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jesse Glick <jesse.gl...@sun.com>
Subject Re: Purpose of FileUtils.close(...)
Date Sat, 18 Dec 2004 00:38:18 GMT
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 <mailto:jesse.glick@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


Mime
View raw message