Use try-with-resources or close this "BufferedReader" in a "finally" clause

Question

Been looking for a way to fix this issue. Read all the previous answers but none helped me out. Could it be any error with SonarQube?

public class Br {

    public String loader(String FilePath){

        BufferedReader br;
        String str = null;
        StringBuilder strb = new StringBuilder();
        try {
            br = new BufferedReader(new FileReader(FilePath));
            while ((str = br.readLine()) != null) {
                strb.append(str).append("\n");
            }
        } catch (FileNotFoundException f){
            System.out.println(FilePath+" does not exist");
            return null;
        } catch (IOException e) {
            e.printStackTrace();
        }
        return strb.toString();
    }
}

Run you code 1 million times. You'll see that you've created a resource leak. You're opening resources, but are never closing them — Lino, May 15 '19 at 13:58
You are missing the `br.close()` method in order to prevent a resource leak. This may be achieved in a `finally` block after the `catch` blocks or in a `try (BufferedReader br = new BufferedReader(new FileReader(FilePath))) { ....` — deHaar, May 15 '19 at 13:58
Wouldn't help at all. Tried everything that was suggested still got plenty of other errors. If I try to add finally{} , I get "br was not initialized" . After I set br to null, I end up in other errors and so on. — Lol, May 15 '19 at 14:04
Imo it is weird you have `return null` inside the first exception, even though your `return strb.toString()` is outside the `try`, and the `IOException` does not have a return null. Either put `return null` inside both exceptions with the `return strb.toString()` inside the try, or leave it outside without the `return null`s. It just makes your code confusing because in IOException you will return strb.toString(). (This is not really related to your other problem though) — Nexevis, May 15 '19 at 14:04

deHaar · Accepted Answer · 2020-10-06T07:54:19.933

18

You are not calling br.close() which means risking a resource leak. In order to reliably close the BufferedReader, you have two options:

using a finally block:

public String loader(String FilePath) {
    // initialize the reader with null
    BufferedReader br = null;
    String str = null;
    StringBuilder strb = new StringBuilder();

    try {
        // really initialize it inside the try block
        br = new BufferedReader(new FileReader(FilePath));
        while ((str = br.readLine()) != null) {
            strb.append(str).append("\n");
        }
    } catch (FileNotFoundException f) {
        System.out.println(FilePath + " does not exist");
        return null;
    } catch (IOException e) {
        e.printStackTrace();
    } finally {
        // this block will be executed in every case, success or caught exception
        if (br != null) {
            // again, a resource is involved, so try-catch another time
            try {
                br.close();
            } catch (IOException e) {
                e.printStackTrace();
            }
        }
    }

    return strb.toString();
}

using a try-with-resources statement:

public String loader(String FilePath) {
    String str = null;
    StringBuilder strb = new StringBuilder();

    // the following line means the try block takes care of closing the resource
    try (BufferedReader br = new BufferedReader(new FileReader(FilePath))) {
        while ((str = br.readLine()) != null) {
            strb.append(str).append("\n");
        }
    } catch (FileNotFoundException f) {
        System.out.println(FilePath + " does not exist");
        return null;
    } catch (IOException e) {
        e.printStackTrace();
    }

    return strb.toString();
}

edited Oct 06 '20 at 07:54

answered May 15 '19 at 14:06

deHaar

17,687
10
38
51

Ok so the method with finally brackets worked out great! Don't know why try-with-resources didn't work out! Thank you very much! – Lol May 15 '19 at 14:18
@Lol You're welcome! That's the classic version. Have you tried the other one? Has SonarQube complained about it? Consider accepting the answer if it was helpful, it would grant you a little reputation, too. – deHaar May 15 '19 at 15:11
The second version doesn;t seem to be working for me! Would give the same error as before. – Lol May 15 '19 at 15:19
@Lol That's interesting... Wasn't it SonarQube itself that stated *Use try-with-resources or close this “BufferedReader” in a “finally” clause*? The title of your question is an error message, isn't it? – deHaar May 15 '19 at 15:22
Yes it was! When tried try-with-resources, I ended up in the same statement which is totally weird. – Lol May 15 '19 at 15:33
@Lol that might be [an already known issue of SonarQube](https://stackoverflow.com/questions/53009113/sonarqube-false-positive-for-use-try-with-resources-or-close-this-resultset-i)... Not our fault ;-) – deHaar May 15 '19 at 15:36
Hi I added try-with-resource to my code, and got error 'StatementIsClosedException: No operations allowed after statement closed', what should I do with this? – Cecilia Nov 06 '19 at 22:04
@Cecilia You cannot access the resource outside the scope of the `try`-with-resources block, but you obviously do it, which leads to this `Exception`. Store the data you need in data structures defined outside that scope. – deHaar Nov 07 '19 at 07:04

TheWhiteRabbit · Answer 2 · 2019-05-15T14:20:59.253

The code you wrote is indeed leaking resources as you're not closing your BufferedReader. The following snippet should do the trick:

public String loader(String filePath){
    String str = null;
    StringBuilder strb = new StringBuilder();
    // try-with-resources construct here which will automatically handle the close for you
    try (FileReader fileReader = new FileReader(filePath); 
         BufferedReader br = new BufferedReader(fileReader);){
        while ((str = br.readLine()) != null) {
            strb.append(str).append("\n");
        }
    }
    catch (FileNotFoundException f){
        System.out.println(filePath+" does not exist");
        return null;
    }
    catch (IOException e) {
        e.printStackTrace();
    }
    return strb.toString();
}

If you're still having issues with this code, then yes, it's SonarQubes fault :-)

Oh ok then it's SonarQubes error! Tried that before.Tried again now and still got same error.Thanks! — Lol, May 15 '19 at 14:10
Perhaps a long shot, but maybe SonarQube requires both the FileReader and BufferedReader to be declared separately in the try-with-resources construct? Adjusted my sample based on this. — TheWhiteRabbit, May 15 '19 at 14:22

score 2 · Answer 3 · answered May 15 '19 at 14:04

Seems like you just want to read all lines from a file. You could use this:

public String loader(String FilePath) {
    try(Scanner s = new Scanner(new File(FilePath).useDelimiter("\\A")) {
        return s.hasNext() ? s.next() : null;
    } catch(IOException e) {
        throw new UncheckedIOException(e);
    }
}

Use try-with-resources or close this "BufferedReader" in a "finally" clause

3 Answers3

Linked