'fileInputStream' is not closed on exit.

Question

I have performed klocwork code analysis on my code. I am getting the following error I have closed the inputsream in finally Even then i am getting the error 'fileInputStream' is not closed on exit.

Following is the piece of code

    LOGGER.log(Level.INFO, "Inside unzipDownloadedFile method");
    File fileDirectory = new File(destDir);
    FileInputStream fileInputStream = null;
    // buffer for read and write data to file
    byte[] buffer = new byte[1024];
    ZipInputStream zipInputStream = null;
    File zipPath = new File(zipFilePath);
    FileOutputStream fileOutputStream = null;
    // create output directory if it doesn't exist
    if (!fileDirectory.exists()) {
        fileDirectory.mkdirs();
    }
    if (zipPath != null) {
        if (zipPath.exists()) {
            try {
                fileInputStream = new FileInputStream(zipFilePath);
                zipInputStream = new ZipInputStream(fileInputStream);
                ZipEntry zipEntry = zipInputStream.getNextEntry();
                while (zipEntry != null) {
                    String fileName = zipEntry.getName();
                    File newFile = new File(destDir + File.separator
                            + fileName);
                    // create directories for sub directories in zip
                    new File(newFile.getParent()).mkdirs();
                    fileOutputStream = new FileOutputStream(newFile);
                    int zipStream = 0;
                    while ((zipStream = zipInputStream.read(buffer)) > 0) {
                        fileOutputStream.write(buffer, 0, zipStream);
                    }
                    fileOutputStream.close();
                    // close this ZipEntry
                    zipInputStream.closeEntry();
                    zipEntry = zipInputStream.getNextEntry();
                }
                fileInputStream.close();
            } catch (IOException ioException) {
                ioException.printStackTrace();
            } finally {
                try {
                    // close last ZipEntry
                    if (zipInputStream != null) {
                        zipInputStream.closeEntry();
                        zipInputStream.close();
                    }
                    if (fileInputStream != null) {
                        fileInputStream.close();
                    }
                    if (fileOutputStream != null) {
                        fileOutputStream.close();
                    }
                } catch (IOException e) {
                    // TODO Auto-generated catch block
                    e.printStackTrace();
                }
            }
        }
    }

Answer 1

It's subtle, but if you look closely at your finally block, you'll see that if there's an exception closing zipInputStream, fileInputStream and fileOutputStream are never closed. And an exception closing fileInputStream will prevent fileOutputStream from being closed. And even though your try has a fileInputStream.close() in it, it may not be reached (because of an exception in the main code).

} finally {
    try {
        // close last ZipEntry
        if (zipInputStream != null) {
            zipInputStream.closeEntry();    // An exception here...
            zipInputStream.close();         // Or here...
        }                                   // Prevents all of the following from running
        if (fileInputStream != null) {
            fileInputStream.close();        // Similarly, an exception here...
        }
        if (fileOutputStream != null) {     // Prevents this from running
            fileOutputStream.close();
        }
    } catch (IOException e) {
        // TODO Auto-generated catch block
        e.printStackTrace();
    }
}

I'd suggest using the new try-with-resources statement.

---

You've asked below:

Is it not possible to use simple try?As i am using java 6.Its the requirement

First off, get whoever it is who gave you that requirement to change it. Perhaps hand them a multi-year calendar and point to July 28th, 2011 — three and a half years ago — which is the date Java 6 was replaced by Java 7, then again at March 18, 2014, when Java 8 replaced Java 7. It's well past time to at least be using Java 7, and Java 7 has try-with-resources.

But yes, of course it was possible to handle this in the 15 years between Java 1 and Java 7 . You just have to handle the close exceptions individually rather than as a group. I used to have helper functions for this in a StreamHelper class. They looked like this:

public static InputStream cleanClose(InputStream in ) {
    if (in != null) {
        try {
            in.close();
        }
        catch (Exception e) {
            // ...do something appropriate, but swallow it...
        }
    }
    return null;
}

...and similarly for OutputStream and such.

Then I'd use it in exception handlers, using this pattern:

InputStream theInput = null;
OutputStream theOutput = null;
try {
    theInput = new FileInputStream(/*...*/);
    theOutput = new FileOutputStream(/*...*/);

    // ...do something with the streams

    // Close them normally
    theOutput.close();
    theOutput = null;
    theInput.close();
    theInput = null;
}
finally {
    // Ensure they're closed
    theOutput = StreamHelper.cleanClose(theOutput);
    theInput = StreamHelper.cleanClose(theInput);
}

Among other things, this let me declare exceptions on detail methods and allow them to propagate, while still handling local streams within the detail method.

try-with-resources is better, though, because exceptions that occur during cleanup while another exception is being raised become part of the main exception being raised (they're called "suppressed" exceptions and they're on the main exception), whereas with old-style code like the above, you rely on logging or your own bespoke mechanism for knowing about them.