Purpose of FileUtils.close(...) (was: Re: [Patch] FileUtils more minor changes)

[Jesse Glick <je...@sun.com>]

Kev Jackson wrote:
> As an aside, I went through the entire codebase last night and replaced 
> every
> try {
>  somthing.close()
> } catch (IOException e) {
>    //swallow exception
> }
> 
> with 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):

OutputStream os = null;
try {
     os = new FileOutputStream(...);
     printStuffToStream(os);
} catch (IOException e) {
     throw new BuildException(e);
} finally {
     FileUtils.close(os);
}

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);
}

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.

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?

-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

Re: Purpose of FileUtils.close(...)

[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

Re: Purpose of FileUtils.close(...)

[Kev Jackson <ke...@it.fts-vn.com>]

>
> 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