views:

197

answers:

3

In my Java code, I start a new process, then obtain its input stream to read it:

BufferedReader reader = new BufferedReader(new InputStreamReader(process.getInputStream()));

FindBugs reports an error here:

may fail to close stream
Pattern id: OS_OPEN_STREAM, type: OS, category: BAD_PRACTICE

Must I close the InputStream of another process? And what's more, according to its Javadoc, InputStream#close() does nothing. So is this a false positive, or should I really close the input stream of the process when I'm done?

+2  A: 

InputStream is an abstract class - just because its implementation does nothing doesn't mean that the actual type of object returned by process.getInputStream() doesn't.

It's possible that failing to close the input stream in this particular case would do no harm - but I personally wouldn't count on it. Close it like you'd close any other input stream. Aside from anything else, that makes your code more robust in case you ever decide to change it to read from something else - it would be all too easy to (say) read from a file instead, and not notice that you're not closing the FileInputStream.

Jon Skeet
What a fail not noticing it being abstract - shame on me...Okay, I'll close it. The thing what got me confused is that it's not me explicitly opening the stream.
thSoft
A: 

I think its always a good practice to close all the streams you open. Preferably in the finally{} block. Since it does nothing as java says, why not call the close() method. Its of no harm.

Bragboy
+3  A: 

In this case, you want to close() the Reader, which will close its underlying streams. Yes, it's always good practice to close streams, even if at the moment you know the implementation you're looking at doesn't do anything (though, in fact, it does here!). What if that changed later?

FindBugs is only there to warn about possible errors; it can't always know for sure.

Finally yes, your Java process owns the process and Process object you spawned. You most definitely need to close that and the output stream. Nobody else is using them, and, it's important to do such things to avoid OS-related stream funny business.

Sean Owen
Okay, another fail from me relying on the implementation... I close the Reader now, as you suggested.
thSoft