2

I know that an InputStream should be closed. But I have some doubts where and how to do this.

According to the documentation on IOUtils.closeQuietly:

Unconditionally close an InputStream. Equivalent to InputStream.close(), except any exceptions will be ignored. This is typically used in finally blocks.

I don't need a try/catch block in my code so I don't have a finally block. Is it fine to just close the InputStream before returning it in my method below or should I do something differently? This method will be used by several services to load an InputStream from a file. 

public InputStream read(String filename) {
    InputStream inputStream = Thread.currentThread().getContextClassLoader().getResourceAsStream(filename);

    if (inputStream == null) {
      // Throw some exception
    }

    IOUtils.closeQuietly(inputStream);

    return inputStream;
}
Diyarbakir
  • 1,999
  • 18
  • 36
  • Looking at your method, it shouldn't be the one to close the `InputStream`. Let the caller close it approprietly. – Tunaki Apr 25 '16 at 12:27
  • Why are you returning the `InputStream` and not the contents of the File? – ndrone Apr 25 '16 at 12:42
  • @ndrone Because there is other logic that follows which needs the file as `InputStream`. But that isn't relevant for the question imo. – Diyarbakir Apr 25 '16 at 12:46
  • I guess I'm just trying to understand what good is a closed `InputSteam` is to the other logic. But it looks like you found your answer – ndrone Apr 25 '16 at 12:53

2 Answers2

9

You shouldn't be calling IOUtils.closeQuietly(inputStream); at all in this method - there is very little point in returning a closed stream.

However, this method should be called in a try/finally block:

InputStream is = null;
try {
  is = read(filename);
  // Do whatever with is.
} finally {
  IOUtils.closeQuietly(is);
}

or with try-with-resources (noting the comment here that "try-with-resources statement will eliminate most needs for using IOUtils.closeQuietly"):

try (InputStream is = read(filename)) {
  // Do whatever with is.
}
Community
  • 1
  • 1
Andy Turner
  • 137,514
  • 11
  • 162
  • 243
  • I have multiple services that need to read an `InputStream` from a file which in turn needs to be mapped to some `Object` using `ObjectMapper`. I want separation of concerns (read from file, map to object) and I want to reuse logic. I could use an Abstract parent class, static Utility class whatever. My point is that my read method is not throwing an Exception at any point so I don't need a try/catch. Therefore I don't have a finally block in there. Should I still just surround it with a try/finally? – Diyarbakir Apr 25 '16 at 12:36
  • "my read method is not throwing an Exception at any point" Sure, maybe, now. What if you change the method? It's just best practice to remove the possibility of not closing a stream. – Andy Turner Apr 25 '16 at 12:39
  • Thanks for the useful tips here. I'll refactor my code to make it better. – Diyarbakir Apr 25 '16 at 12:49
0

try/finally block:

InputStream is = null; try { InputStream is = is = read(filename); // Do whatever with is. } finally { is.close(); }

Note: all i/o resources need to be closed in the finally block since it was initialized in the try-catch block. It is also advised to add:

} catch(IOException e){
   e.printstacktrace();
}

...for exception handling

Ashish Patel
  • 163
  • 6