Resource leak while using try...finally?

Question

I was working normally in eclipse when I got bugged by a resource leak warning in both return values inside the try block in this method:

@Override
public boolean isValid(File file) throws IOException
{
    BufferedReader reader = null;
    try
    {
        reader = new BufferedReader(new FileReader(file));
        String line;
        while((line = reader.readLine()) != null)
        {
            line = line.trim();
            if(line.isEmpty())
                continue;
            if(line.startsWith("#") == false)
                return false;
            if(line.startsWith("#MLProperties"))
                return true;
        }
    }
    finally
    {
        try{reader.close();}catch(Exception e){}
    }
    return false;
}

I don't understand how it would cause resource leak since I'm declaring the reader variable outside the try scope, adding a resource inside the try block and closing it in a finally block using an other try...catch to ignore exceptions and a NullPointerException if reader is null for some reason...

From what I know, finally blocks are always executed when leaving the try...catch structure, so returning a value inside the try block would still execute the finally block before exiting the method...

This can be easily proved by:

public static String test()
{
    String x = "a";
    try
    {
        x = "b";
        System.out.println("try block");
        return x;
    }
    finally
    {
        System.out.println("finally block");
    }
}

public static void main(String[] args)
{
    System.out.println("calling test()");
    String ret = test();
    System.out.println("test() returned "+ret);
}

It result in:

calling test()
try block
finally block
test() returned b

Knowing all this, why is eclipse telling me Resource leak: 'reader' is not closed at this location if I'm closing it in my finally block?

---

Answer

I would just add to this answer that he's correct, if new BufferedReader throws an exception, an instance of FileReader would be open upon destruction by garbage collector because it wouldn't be assigned to any variable and the finally block would not close it because reader would be null.

This is how I fixed this possible leak:

@Override
public boolean isValid(File file) throws IOException
{
    FileReader fileReader = null;
    BufferedReader reader = null;
    try
    {
        fileReader = new FileReader(file);
        reader = new BufferedReader(fileReader);
        String line;
        while((line = reader.readLine()) != null)
        {
            line = line.trim();
            if(line.isEmpty())
                continue;
            if(line.startsWith("#") == false)
                return false;
            if(line.startsWith("#MLProperties"))
                return true;
        }
    }
    finally
    {
        try{reader.close();}catch(Exception e){}
        try{fileReader.close();}catch(Exception ee){}
    }
    return false;
}

Answer 1

There is technically a path for which the BufferedReader would not be closed: if reader.close() would throw an exception, because you catch the exception and do nothing with it. This can be verified by adding reader.close() again in your catch block:

    } finally
    {
        try {
            reader.close();
        } catch (Exception e) {
            reader.close();
        }
    }

Or by removing the try/catch in the finally:

    } finally
    {
            reader.close();
    }

This will make the warnings disappear.

Of course, it doesn't help you. If reader.close() is failing, then calling it again does not make sense. The thing is, the compiler is not smart enough to handle this. So the only sensible thing you can do is to add a @SuppressWarnings("resource") to the method.

Edit If you are using Java 7, what you can/should do is using try-with-resources functionality. This will get the warnings right, and makes you code simpler, saving you a finally block:

public boolean isValid(File file) throws IOException
{
  try(BufferedReader reader = new BufferedReader(new FileReader(file)))
  {
    String line;
    while ((line = reader.readLine()) != null)
    {
      line = line.trim();
      if (line.isEmpty())
        continue;
      if (line.startsWith("#") == false)
        return false;
      if (line.startsWith("#MLProperties"))
        return true;
    }
  } 
  return false;
}

Answer 2

If the BufferedReader constructor throws an exception (e.g. out of memory), you will have FileReader leaked.

Answer 3

//If this line throws an exception, then neither the try block
    //nor the finally block will execute.
    //That is a good thing, since reader would be null.
    BufferedReader reader = new BufferedReader(new FileReader(aFileName));
    try {
      //Any exception in the try block will cause the finally block to execute
      String line = null;
      while ( (line = reader.readLine()) != null ) {
        //process the line...
      }
    }
    finally {
      //The reader object will never be null here.
      //This finally is only entered after the try block is 
      //entered. But, it's NOT POSSIBLE to enter the try block 
      //with a null reader object.
      reader.close();
    }

Answer 4

Since close() can throw an exception (why oh why did they design it that way...) I tend to use a double try

try {
  BufferedReader reader = new BufferedReader(new FileReader(file));
  try {
    // do stuff with reader
  } finally {
    reader.close();
  }
} catch(IOException e) {
  // handle exceptions
}

Since this idiom eliminates the try/catch within the finally block it may be enough to keep Eclipse happy.

new BufferedReader(...) can't itself throw an IOException but technically this could still leak the FileReader if the BufferedReader constructor throws a RuntimeException or Error.