11

This piece of code is creating memory leak issues cause of BufferedReader and InputStreamReader which I think might be happening cause of some exceptions. How should I change it? 

try{
    URL url = new URL(sMyUrl);
    BufferedReader in = new BufferedReader(new InputStreamReader(url.openStream()));
    while ((str = in.readLine()) != null) {
        jsonString += str;
    }
    in.close();
}catch(Exception e){

}
Aske B.
  • 6,419
  • 8
  • 35
  • 62
Pit Digger
  • 9,618
  • 23
  • 78
  • 122
  • 1
    May be move close() logic to finally block? Are you sure no exceptions happening while reading (or) closing connection? – kosa Aug 30 '12 at 14:27
  • Have tried writing `e.printStackTrace()` in your catch-clause to see if exceptions where thrown? – Aske B. Aug 30 '12 at 14:29

2 Answers2

17

It would be safer to close your stream using a try..finally block. You might also use a StringBuilder as it is designed for concatenating strings. You should also avoid catching Exception and doing nothing with it. Also, your code is concatenating lines without any line-breaks. This may well not be what you want, in which case append("\n") when you read each line in.

Here's a version with those modifications:

StringBuilder json = new StringBuilder();
try {
    URL url = new URL(sMyUrl);
    BufferedReader in = new BufferedReader(new InputStreamReader(url.openStream()));
    try {
        String str;
        while ((str = in.readLine()) != null) {
            json.append(str).append("\n");
        }
    } finally {
        in.close();
    }
} catch (Exception e) {
    throw new RuntimeException("Failed to read JSON from stream", e);
}
ᴇʟᴇvᴀтᴇ
  • 12,285
  • 4
  • 43
  • 66
  • Do I need to close InputStreamReader ? Or its closed as Buffer reader will create a new object? – Pit Digger Aug 30 '12 at 14:35
  • 10
    No need to explicitly close the `InputStreamReader` as it will be closed automatically when you close the `BufferedReader`. – ᴇʟᴇvᴀтᴇ Aug 30 '12 at 14:36
  • @aetheria, thank you. Anyhow I need a good Java streams tutorial. – Maksim Dmitriev Feb 18 '13 at 08:02
  • @aetheria I have seen in some tutorials, the resource is being checked for null ness before closing. But when I try that one it asked me to enclosed them in another try catch block. why is that? and is it essential to check for null ness before closing a resource – Kasun Siyambalapitiya Jan 17 '17 at 12:03
  • Kasun, I suggest you post a separate question with an example. – ᴇʟᴇvᴀтᴇ Jan 17 '17 at 14:28
  • This answer is incorrect as it doesn't even compile (at least in java7). Variable `BufferedReader in` is declared in scope of `try` block and is not visible in scope of `finally` block. The variable may not even get assigned in case `MalformedURLException` is thrown from URL class constructor. – Krzysztof Jabłoński Apr 03 '18 at 16:31
  • No, this is a misreading. There are two `try` blocks. The variable `in` is not defined inside the `try` block that closes it. – ᴇʟᴇvᴀтᴇ Apr 05 '18 at 16:33
  • @ᴇʟᴇvᴀтᴇ You're right. Something must be wrong with me then. Sorry for confusion. – Krzysztof Jabłoński May 07 '18 at 14:27
12

The code isn't pretty but won't be creating a memory leak. I suggest you use a memory profiler to determine where your memory is being used. Otherwise you are just guessing even if you have ten + years experience performance tuning in Java ;)

A better alternative is to use Java 7

URL url = new URL(sMyUrl);
try(BufferedReader in = new BufferedReader(new InputStreamReader(url.openStream()))) {
  while ((str = in.readLine()) != null) {
     jsonString.append(str).append("\n");
  }
}

If you have Java 6 or older you can use.

BufferedReader in = new BufferedReader(new InputStreamReader(url.openStream()))) {
try {
  while ((str = in.readLine()) != null) {
     jsonString.append(str).append("\n");
  }
} finally {
  in.close();
}
Peter Lawrey
  • 525,659
  • 79
  • 751
  • 1,130