How do I close a file after catching an IOException in java?

Question

All,

I am trying to ensure that a file I have open with BufferedReader is closed when I catch an IOException, but it appears as if my BufferedReader object is out of scope in the catch block.

public static ArrayList readFiletoArrayList(String fileName, ArrayList fileArrayList)
{
    fileArrayList.removeAll(fileArrayList);

    try {
        //open the file for reading
        BufferedReader fileIn = new BufferedReader(new FileReader(fileName));

        // add line by line to array list, until end of file is reached
        // when buffered reader returns null (todo). 
        while(true){
                fileArrayList.add(fileIn.readLine());
            }
    }catch(IOException e){
        fileArrayList.removeAll(fileArrayList);
        fileIn.close(); 
        return fileArrayList; //returned empty. Dealt with in calling code. 
    }
}

Netbeans complains that it "cannot find symbol fileIn" in the catch block, but I want to ensure that in the case of an IOException that the Reader gets closed. How can I do that without the ugliness of a second try/catch construct around the first?

Any tips or pointers as to best practise in this situation is appreciated,

score 25 · Accepted Answer · answered Apr 15 '10 at 22:09

25

 BufferedReader fileIn = null;
 try {
       fileIn = new BufferedReader(new FileReader(filename));
       //etc.
 } catch(IOException e) {
      fileArrayList.removeall(fileArrayList);
 } finally {
     try {
       if (fileIn != null) fileIn.close();
     } catch (IOException io) {
        //log exception here
     }
 }
 return fileArrayList;

A few things about the above code:

close should be in a finally, otherwise it won't get closed when the code completes normally, or if some other exception is thrown besides IOException.
Typically you have a static utility method to close a resource like that so that it checks for null and catches any exceptions (which you never want to do anything about other than log in this context).
The return belongs after the try so that both the main-line code and the exception catching have a return method without redundancy.
If you put the return inside the finally, it would generate a compiler warning.

answered Apr 15 '10 at 22:09

Yishai

90,445
31
189
263

+1 for the `finally`. Surprisingly a lot of answers didn't mention about it. Here are some Sun links of interest: [Lesson: Basic IO](http://java.sun.com/docs/books/tutorial/essential/io/) and [Lesson: Exceptions](http://java.sun.com/docs/books/tutorial/essential/exceptions/index.html) – BalusC Apr 15 '10 at 22:13
You can remove the check for `null` and catch `Exception`, unless you plan on telling the user that you couldn't close the file versus couldn't open it. But, generally, users don't care. – Dave Jarvis Apr 15 '10 at 22:27
You closed the BufferdedRead, but not the FileReader? – Bert F Apr 15 '10 at 22:43
1

Closing the BufferedReader will automatically close the FileReader. – DaveJohnston Apr 15 '10 at 23:09
1

@Bert: Indeed, the Java IO classes follows the decorator pattern. So will under each `close()` delegate upwards to the decorated instance. – BalusC Apr 16 '10 at 02:10
@Dave Jarvis, it is certainly a reasonable alternative, but I think the above represents the "standard" best practice. But as long as you log the exception, then I think it doesn't really matter. – Yishai Apr 16 '10 at 02:59
This is not considered best practice - rather `Resource resource = acquire(); try { use(resource); } finally { resource.release(); }` - please have a look at my answer [below](http://stackoverflow.com/a/17734143/281545) and links there – Mr_and_Mrs_D Jul 18 '13 at 21:35
@Mr_and_Mrs_D, the problem with that advice is that the acquire generally throws the same exception as the subsequent operations, causing messiness in your exception handling. If you regard an exception on the close method as equivalent to the other exceptions, then it doesn't matter, but if you want to handle them separately, well, checking for null is just the cleanest thing to do. – Yishai Jul 18 '13 at 21:55
You mean `Resource resource = acquire(); try { use(resource); } finally { try { resource.release(); } catch { // log and swallow} }` as I did, cause usually the `close()` exception is not very important (not always - follow links) – Mr_and_Mrs_D Jul 18 '13 at 22:00
Is it just me, but putting a try within a catch for the _same_ exception seems bizarrely redundant. I understand that we have to do it, but it screams that something fundamental is seriously flawed. – SMBiggs Jun 28 '16 at 20:15
This is the old way of doing things. The new (well.... Java 7) feature to automatically close resources should be added first in this answer as the preferred way. – Klas Lindbäck Jan 15 '18 at 07:32

score 1 · Answer 2 · answered Apr 15 '10 at 22:08

1

Once you hit the catch block, any variables declared in the try are not scoped anymore. Declare BufferedReader fileIn = null; above the try block, then assign it inside. In your catch block, do if(fileIn != null) fileIn.close();

answered Apr 15 '10 at 22:08

purecharger

1,213
2
15
34

score 1 · Answer 3 · answered Apr 15 '10 at 22:08

It's complaining about the symbol not being there because it's not. It's in the try block. If you want to refer to fileIn, you'll need to declare it outside the try.

However, it really sounds like you'd want to place the close in a finally block instead: you should close the file regardless of success or failure before returning.

public static ArrayList readFiletoArrayList(String fileName, ArrayList fileArrayList)
{
    fileArrayList.removeAll(fileArrayList);

    BufferedReader fileIn = null;
    try {
        //open the file for reading
        fileIn = new BufferedReader(new FileReader(fileName));

        // add line by line to array list, until end of file is reached
        // when buffered reader returns null (todo). 
        while(true){
                fileArrayList.add(fileIn.readLine());
            }
    }catch(IOException e){
        fileArrayList.removeAll(fileArrayList); 
    }finally{
        if(fileIn != null) fileIn.close();
    }
    return fileArrayList;
}

score 1 · Answer 4 · answered Apr 15 '10 at 22:16

My preferred way of performing clean-up after an exception (when the clean-up can potentially also throw an exception) is to put the code in the try block inside another try/finally block, as follows:

public static ArrayList readFiletoArrayList(String fileName, ArrayList fileArrayList) {
    fileArrayList.removeAll(fileArrayList);

    try {
        //open the file for reading
        BufferedReader fileIn = null;

        try {
            fileIn = new BufferedReader(new FileReader(fileName));
            // add line by line to array list, until end of file is reached
            // when buffered reader returns null (todo). 
            while(true){
                fileArrayList.add(fileIn.readLine());
            }
        } finally {
            if (fileIn != null) {
                fileIn.close();
            }
        }
    }catch(IOException e){
        fileArrayList.removeAll(fileArrayList);
        return fileArrayList; //returned empty. Dealt with in calling code. 
    }
}

This has the disadvantage of swallowing the wrong exception. If the readLine fails for some reason it throws an IOException. The file is then closed inside the finally. If the file close also fails, a new IOException is thrown and the first exception, with a possibly valuable error message, is lost. — Florian F, Mar 03 '17 at 09:04

score 0 · Answer 5 · edited May 23 '17 at 12:17

0

It's better not to deal with null - the general idiom for resource acquisition and release in Java is :

final Resource resource = acquire();
try { use(resource); }
finally { resource.release(); }

so:

public static List<String> readFiletoArrayList(String fileName,
        List<String> fileArrayList, String charsetName) {
    fileArrayList.clear(); // why fileArrayList.removeAll(fileArrayList) ?
    try {
        InputStream file = new FileInputStream(fileName);
        try {
            InputStreamReader reader = new InputStreamReader(file, charsetName);
            BufferedReader buffer = new BufferedReader(reader);
            for (String line = buffer.readLine(); line != null; line = buffer
                    .readLine()) {
                fileArrayList.add(line);
            }
        } finally {
            try {
                file.close();
            } catch (IOException e) {
                e.printStackTrace(); // you do not want this to hide an
                // exception thrown earlier so swallow it
            }
        }
    } catch (IOException e) {
        fileArrayList.clear(); // returned empty. Dealt with in client
    }
    return fileArrayList;
}

See my points here

If you are using a reader you must specify the encoding as I am doing here. If you want to read bytes forget about the reader. Also if you use readLine() you must forget about the end of line characters - if this is a concern consider omitting the BufferedReader entirely.

edited May 23 '17 at 12:17

Community

1
1

answered Jul 18 '13 at 21:21

Mr_and_Mrs_D

32,208
39
178
361

It would seem to me that if the file was open for writing rather than reading, safe operation would require that any IOException which occurs while closing the file not be swallowed unless there was already an IOException percolating up the call stack. It may be reasonable to swallow an `IOException` thrown by a file-close operation which is itself within a `Catch(IOException e)`, but if the `finally` is reached via normal code execution, swallowing an `IOException` on close could cause the caller to erroneously believe a file was written when it actually wasn't. – supercat Jul 26 '13 at 21:55
@supercat: you are right - but this is file reading. Writing must flush the decorator (and not rely on close) - _if there is a decorator_. I go through some peculiarities [here](http://stackoverflow.com/questions/16363642/java-6-io-wrapped-streams-closing) - unjustly closed – Mr_and_Mrs_D Jul 26 '13 at 22:02
1

The pattern of stifling cleanup exceptions is often applied without regard for what the cleanup entails. In some cases it's okay, but as a habit it seems dangerous. As for your linked (closed) question, I'd suggest that you start with a bit less "backstory". What you really want to know (from what I can tell) is how to deal with an exception thrown from a constructor which is supposed to acquire ownership if a passed-in stream. My suggestion would be that such constructors should close the passed-in stream if they throw an exception, or else be wrapped in a factory method that does so. – supercat Jul 26 '13 at 22:09
While necessary, that is some ugly code. What should be a few lines has turned into a mess of scopes with a tangled spaghetti flow. And to think all this complexity was added for the sake of simplicity! Makes me yearn for the days of assembly. – SMBiggs Jun 28 '16 at 19:18

score 0 · Answer 6 · answered Apr 15 '10 at 22:07

Move the declaration out of the try block:

public static ArrayList readFiletoArrayList(String fileName, ArrayList fileArrayList)
{
    fileArrayList.removeAll(fileArrayList);

    BufferedReader fileIn = null;
    try {
        //open the file for reading
        fileIn = new BufferedReader(new FileReader(fileName));

        // add line by line to array list, until end of file is reached
        // when buffered reader returns null (todo). 
        while(true){
                fileArrayList.add(fileIn.readLine());
            }
    }catch(IOException e){
        fileArrayList.removeAll(fileArrayList);
        fileIn.close(); 
        return fileArrayList; //returned empty. Dealt with in calling code. 
    }
}

But, you'll still need to be careful that fileIn was actually initialized before trying to close it:

if (fileIn != null)
    fileIn.close();

If there's an exception in the reader constructors (file not found, for instance), that'll blow up with a `NullPointerException`. Have to check the `null` in the `catch`. — T.J. Crowder, Apr 15 '10 at 22:10
In your catch, you have `fileIn.close();` which also throws an IOException. The compiler won't allow it. — SMBiggs, Jun 28 '16 at 19:13

score 0 · Answer 7 · answered Apr 15 '10 at 22:15

Declare the BufferedReader outside the try block and set it to null then use a finally block to close it if its not null. Also fileArrayList is passed by reference so any changes made to it will happen to the object you passed in so there is no need to also return it.

    public static ArrayList readFiletoArrayList(String fileName, ArrayList fileArrayList)
{
    fileArrayList.removeAll(fileArrayList);
    BufferedReader fileIn = null;
    try {
        //open the file for reading
        fileIn = new BufferedReader(new FileReader(fileName));

        // add line by line to array list, until end of file is reached
        // when buffered reader returns null (todo). 
        while(true){
                fileArrayList.add(fileIn.readLine());
            }
    }catch(IOException e){
        fileArrayList.removeAll(fileArrayList);  
    }finally
    {
       try
       {
           if(fillIn != null)
               fileIn.close();
       }
       catch(IOException e){}
    }
    return fileArrayList; //returned empty. Dealt with in calling code.
}

How do I close a file after catching an IOException in java?

7 Answers7

Linked