Java resource management: please help to understand Findbugs results.

Question

Hello, everyone!

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?

Answer 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.
    }
}

Answer 2

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

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

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