Will i need to close FileInputStream manually in this code? [duplicate]

Question

I am facing issues with one of my application, and encountered the following exception when handling large amount of transaction that involves opening file and reading the content:

java.io.FileNotFoundException: /keystore-sample/keystore.jks (Too many open files)

I have the following sample code written in java:

try {  
    EncryptDecryptPassword crypt = new EncryptDecryptPassword();
    String secretKey = "secret123";
    String trustpassencrypted = "ecnrypted1234";
    String trustpass = crypt.decryptPassword(trustpassencrypted.trim(), secretKey.trim());

    File truststore = new File("/keystore-sample/keystore.jks");      
    KeyStore keyStore = KeyStore.getInstance("JKS");    
    keyStore.load(new FileInputStream(truststore), trustpass.toCharArray());
    //Continue with the rest of the code...

} catch (KeyStoreException kse) {            
    System.out.println("Security configuration failed with the following: " + kse.getCause());        
} catch (NoSuchAlgorithmException nsa) {            
    System.out.println("Security configuration failed with the following: " + nsa.getCause());        
} catch (FileNotFoundException fnfe) {            
    System.out.println("Security configuration failed with the following: " + fnfe.getCause());        
} catch (UnrecoverableKeyException uke) {            
    System.out.println("Security configuration failed with the following: " + uke.getCause());        
} catch (CertificateException ce) {            
    System.out.println("Security configuration failed with the following: " + ce.getCause());        
}catch (IOException ioe) {            
    System.out.println("Security configuration failed with the following: " + ioe.getCause());        
}

As you can see, i did not declare a new instance for FileInput stream, like the following:
Correction : My intention is to express that i did not assign the newly created FileInputStream into a variable and manually close it later, like the following :

FileInputStream in = new FileInputStream(truststore);
keyStore.load(in, trustpass.toCharArray());
//once done with the input stream, close it
in.close();

My question is : will new FileInputStream(truststore) actually requires manual closing or it will be handled by the underlying Keystore class? Quick glance on the underlying decompiled code of Keystore.class, i don't see that. Just to confirm if this is the particular reason why i am hitting the exception.

Also, is the code implementation above considered as bad practice ?

EDIT:

Due to some restriction on application environment i am running, this is using old Java SE 6u34.

Answer 1

I did not declare a new instance for FileInput stream

This statement is meaningless. There is no such thing as 'declaring a new instance'. You have created a new instance, you just haven't declared a reference variable to store it in. That doesn't relieve you of the new instance, or of the responsibility for closing it.

My question is : will new FileInputStream(truststore) actually requires manual closing

Yes.

or it will be handled by the underlying Keystore class?

No.

Quick glance on the underlying decompiled code of Keystore.class, i don't see that. Just to confirm if this is the particular reason why i am hitting the exception.

'Too many open files': you are leaking file descriptors, either here or elsewhere.

Also, is the code implementation above considered as bad practice?

Yes, because you're not closing the FileInputStream at all. And note that closing it in the line after keyStore.load() isn't sufficient, as that could throw an exception.

These days you would use a try-with-resources:

File truststore = new File("/keystore-sample/keystore.jks");      
try (FileInputStream fis = new FileInputStream(truststore))
{
    KeyStore keyStore = KeyStore.getInstance("JKS");    
    keyStore.load(fis, ...);
}

which would automatically close it for you even if there is an exception.

Answer 2

The fact that you do not "declare a new instance" is irrelevant - the key point is that you create an instance via new. Understanding that is key - even if you don't keep a handle to that instance by assigning it to a variable, you still created it.

In general, if you create it, it's your responsibility to close it. Since you say you can't use Java 7's try-with-resources, you can use the older pattern of closing the created resource in a finally block.

InputStream trust = null;
try {   
    EncryptDecryptPassword crypt = new EncryptDecryptPassword();
    String secretKey = "secret123";
    String trustpassencrypted = "ecnrypted1234";
    String trustpass = crypt.decryptPassword(trustpassencrypted.trim(), secretKey.trim());

    File truststore = new File("/keystore-sample/keystore.jks");      
    KeyStore keyStore = KeyStore.getInstance("JKS");    
    trust = new FileInputStream(truststore)
    keyStore.load(trust, trustpass.toCharArray());
    //Continue with the rest of the code... 
} catch (Exception e) {     
    // note that logging the stack trace is generally a better practice!       
    System.out.println("Security configuration failed with the following: " + e.getCause());        
} finally {
    try { // because close can throw an exception
        if (trust != null) trust.close();
    } catch (IOException ignored) {}
}

Additionally, since you're doing the same thing with all the exceptions, the only reason to list them all is if you feel that it adds to the readability of the code (personally, I don't) by making it explicit what Exceptions you're expecting. Alternatively, again depending what version of Java you're using, you could list them all in the catch block, separated by |.