ant-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kev Jackson <kevin.jack...@it.fts-vn.com>
Subject Re: Purpose of FileUtils.close(...)
Date Fri, 17 Dec 2004 02:48:24 GMT
>
> 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


Mime
View raw message