3

Findbugs bugs me about a method which opens two Closeable instances, but I can't understand why.

Source

public static void sourceXmlToBeautifiedXml(File input, File output) throws TransformerException, IOException, JAXBException { FileReader fileReader = new FileReader(input); FileWriter fileWriter = new FileWriter(output); try { // may throw something sourceXmlToBeautifiedXml(fileReader, fileWriter); } finally { try { fileReader.close(); } finally { fileWriter.close(); } } }

Findbugs analysis

Findbugs tells me

Method [...] may fail to clean up java.io.Reader [...]

and points to the line with FileReader fileReader = ...

Question

Who is wrong: me or Findbugs?

Improve this question

4 Answers 4

Reset to default
4

FindBugs is correct: If the FileWriter's constructor throws an exception, the file reader will not be closed. To verify this, try passing an invalid filename for output.

I'd do it as follows:

 FileReader fileReader = new FileReader(input); try { FileWriter fileWriter = new FileWriter(output); try { // may throw something sourceXmlToBeautifiedXml(fileReader, fileWriter); } finally { fileWriter.close(); } } finally { fileReader.close(); }

Note that the handling of exception thrown when closing could be improved, since leaving a finally-block by throwing an exception will cause the try-statement to terminate by throwing that exception, swallowing any exception thrown in the try-block, which generally would be more useful for debugging. See duffymo's answer for a simple way on how to avoid this.

Edit: Since Java 7, we can use the try-with-resources statement, which permits correct and concicse handling of these corner cases:

try ( FileReader fileReader = new FileReader(input); FileWriter fileWriter = new FileWriter(output) ) { // may throw something sourceXmlToBeautifiedXml(fileReader, fileWriter); }
Improve this answer
Sign up to request clarification or add additional context in comments.

4 Comments

+1 for being the only person to note that the constructor can throw. Personally, I'd call the ctors inside the try (explicitly initializing the variables to null outside it), then use Jakarta IOUtils.closeQuietly() inside the finally.
The actual implementations of FileReader and FileWriter are so broken that they can throw an exception after opening the file (and because it is from the constructor, you don't have a reference to close it yourself). Also note, you are implicitly using whatever charset encoding happens to be configured as the default at the time, which is never a great idea.
Nice - I'd +1 weren't for the fact that exception handling could be improved not only cause throwing from finally would swallow a potential try exception but also cause close() exceptions may very well be significant.
Where did I say they weren't significant? My code doesn't suppress them, does it?
3

This may be complicated even for findbugs.

try { fileReader.close(); } finally { fileWriter.close(); }

Seems to me you are right.

EDIT : Wow, I thought I will get voted down for saying findbugs can be wrong!

EDIT : Looks like FindBugs is right after all. Good catch meriton.

Improve this answer

1 Comment

Well, if you had said that findbugs is wrong I'd probably have downvoted, but you are certainly correct in saying that it can be.
0

i'd say it's you.

i'd close both resources in a separate try/catch block. i'd create static methods to help me:

public static void sourceXmlToBeautifiedXml(File input, File output) throws TransformerException, IOException, JAXBException { FileReader fileReader = new FileReader(input); FileWriter fileWriter = new FileWriter(output); try { // may throw something sourceXmlToBeautifiedXml(fileReader, fileWriter); } finally { close(fileReader); close(fileWriter); } } // same for reader & writer public static void close(InputStream s) { try { if (s != null) { s.close(); } } catch (IOException e) { e.printStackTrace(); } }
Improve this answer

4 Comments

Hmm, I can't believe that this is a common pattern when closing multiple resources. Is there some pattern for this?
There sure is for JDBC. It's common for ResultSet, Statement, and Connection.
I don't know if it's a "pattern", but that static method is the way I write it.
This answer is only confusing - does not address the real issue which is addressed in the accepted answer. Please consider editing it -1
-1

I think findbugs is right.

} finally { try { fileReader.close(); } finally { fileWriter.close(); } }

In this block you try to close your FileReader. This however can throw an exception and in the nested finally you close the fileWriter. Have you tried closing both readers in the same finally block? What does findbugs say then?

} finally { try { fileReader.close(); fileWriter.close(); } finally { //dunno maybe log that something went wrong. } }
Improve this answer

4 Comments

if the fileReader.close() throws an IOException, fileWriter will never be closed.
Your critic applies to your second code example. -- Yes fileReader.close() may throw IOExceptioon, that's why in my example fileWriter.close is in a finally block: if closing fileReader fails, fileWriter gets closed anyway.
Darn. Got me. Rule of thumb: never go to stackoverflow before your first coffee ;-)
Please delete this - does not add any value -1

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.