Is this a memory leak or a false positive?

Question

This is my code:

import java.io.BufferedReader;
import java.io.FileNotFoundException;
import java.io.FileReader;
import java.util.Scanner;

public class temp {
    public static void main(String[] args) throws FileNotFoundException {
        BufferedReader a = new BufferedReader(new FileReader("a"));
        Scanner scanner = new Scanner(a).useDelimiter(",");
        scanner.close();
    }
}

I get a warning at new Scanner(a) that says (I'm compiling with jdk1.7.0_05.):

Resource leak: '<unassigned Closeable value>' is never closed.

Am I doing something wrong, or is this just a false warning?

score 68 · Accepted Answer · edited May 06 '13 at 21:41

68

If you split the code like this, will the warning go away?

  Scanner scanner = new Scanner(a);
  scanner.useDelimiter(",");
  scanner.close();

edited May 06 '13 at 21:41

ajon

7,868
11
48
86

answered Jul 13 '12 at 02:30

Francis Upton IV

19,322
3
53
57

1

Yes, the warning does go away when I split the code like that. So is my original code a memory leak? – dln385 Jul 13 '12 at 02:32
4

No, it's not a leak, it's something the compiler does not detect properly. – Francis Upton IV Jul 13 '12 at 02:32
8

It is a leak, if the implementation of useDelimiter returns something else than "this" (what it doesn't in current implementations). – Bananeweizen Jul 13 '12 at 05:48
I had assumed that `useDelimiter()` returned `this` because that's pretty much an idiom for these sorts of things. But certainly you are right. – Francis Upton IV Jul 13 '12 at 09:44
@Bananeweizen The documentation for `useDeliminiter` explicitly states that it returns `this scanner`, so there won't be a leak in any valid Java implementation. – Voo Jul 13 '12 at 12:01
1

@Bananeweizen, if the implementation of `useDelimiter` returns something other than `this`, then even this solution will be a leak. The returned value of `useDelimiter` is never closed though. Bad enough, there are no way to prevent this leak without double close or similar bad things if you do not have the knowledge what the function returns. – Earth Engine Nov 29 '12 at 00:31
5

Actually, `useDelimiter` has a bad name and interface. If it were called `setDelimiter` and with return type `void` as other similar methods do, it won't be a problem at all. – Earth Engine Mar 15 '13 at 12:13

score 13 · Answer 2 · edited Jul 14 '12 at 00:24

Yes, your code has a potential (but not real) memory leak. You assign the return value of useDelimiter(a) to the local variable scanner, but the constructor result is thrown away. That is why you get the warning.

In practice, the return value of useDelimiter(a) is exactly the same object as the one returned from the constructor call, so your code closes the resource just fine. But this is something the compiler/code analysis tool cannot detect as it would have to know the useDelimiters implementation for that.

And a really good code analysis tool should have shown you an additional warning, because you are closing a resource which has not been opened in this method (the return value of useDelimiter). If you had those 2 messages together, the symptoms might have been more clear to you.

score 1 · Answer 3 · answered Jul 13 '12 at 07:07

1

Have you try :

Scanner scanner = new Scanner(new BufferedReader(new FileReader("a"))).useDelimiter(",");

If it does not work you have to add a.close();

answered Jul 13 '12 at 07:07

cl-r

1,264
1
12
26

I still get the warning when I put it all on one line. – dln385 Jul 13 '12 at 12:23
Try to remove `throws FileNotFoundException` in `main()` and add a `try/cath` block around `BufferedReader`, with `buff.close()` in `finally`. – cl-r Jul 13 '12 at 12:32

score 0 · Answer 4 · answered Jul 13 '12 at 02:33

0

What is giving you this warning? Presumably it warns because the allocation/closing isn't done in a try/finally block, which generally is a bad idea (in this specific case not a problem though because the only thing that can throw an error is the new FileReader and if it throws no resource was actually allocated - but that can change with a single method call..)

Closing a scanner closes the underlying stream (to be exact anything that implements itself Closeable (yes BufferedReader does) so the code is fine apart from that.

answered Jul 13 '12 at 02:33

Voo

29,040
11
82
156

I've tried putting it in a try block with the close in a finally block, and I still get the warning. – dln385 Jul 13 '12 at 02:34
@din In that case it's an error of the static analyser whichever you're using. – Voo Jul 13 '12 at 02:35
In this case a try block will not be helpful, since if there is an exception in the statement allocating the scanner, there is nothing to close. – Francis Upton IV Jul 13 '12 at 02:35
@Francis Yes we know that, but there are cases where it's impossible for the tool (or the programmer) to decide whether any runtime exception can happen, so warning in all cases seems like an easy solution to avoid false negatives. – Voo Jul 13 '12 at 02:37

Is this a memory leak or a false positive?

4 Answers4

Linked

Related