3

If I have stream (InputStream or OutputStream) which I did not create but was rather passed to my method as a parameter, should I be closing that stream? Here's an example:

void method(InputStream in) {
    try {
    //Do something
    }
    finally {
        if(in != null) { 
        in.close();    //Is this needed and correct?
    }    
}
sdm
  • 1,236
  • 11
  • 9
  • 14
    No, you shouldn't. – Andy Turner Oct 26 '17 at 12:03
  • Either way you should declare in the methods *JavaDoc comment* if you close the resource or not. – Timothy Truckle Oct 26 '17 at 12:05
  • @sdm I could be wrong but if you wrote the `method`, you would have some background on the context in which an `InputStream` would be passed to your method to be able to make correct use of it in your method? If you don't have an idea about how your method would be called, isint it prone to side effects considering the stream would be used elsewhere as well? – Chetan Kinger Oct 26 '17 at 12:18
  • Possible duplicate of [Does argument/parameters InputStream need to be closed in android?](https://stackoverflow.com/questions/23925151/does-argument-parameters-inputstream-need-to-be-closed-in-android) – Not a bug Oct 26 '17 at 12:25

5 Answers5

3

Really, "it depends".

As a general rule, you should not close a stream that you didn't have responsibility for opening, but to give a correct answer we would have to understand the context.

It's very possible that the delegation of responsibility requires your method to consume from and close the stream - if this is the case then it should be explicit in the code.

If your method is named readFromStreamAndClose(InputStream in) then the fact that your method closes the stream is very obvious.

In the case that you open the stream yourself, you can always use a try-with-resources block which will close the stream for you - at the same level of abstraction as it was created. In this case - your method (which is called at a lower level than when the stream was opened) should not close the stream.

vikingsteve
  • 38,481
  • 23
  • 112
  • 156
  • I agree that if the API doc does say that the stream passed to it should be closed then as an implementer I do have to close it to conform with the API. That's why I had this question because while writing the implementation, it felt weird being mandated to close the stream which I did not create and I can think of scenarios where this stream may be needed afterwards. – sdm Oct 27 '17 at 11:21
  • Mostly definitely you are correct sdm. Given the behaviour is mandated in this case - make it very clear via documentaiton. – vikingsteve Oct 27 '17 at 12:14
1

Generally it is not recommended to close the stream which is not associated to that class.

Following are the reasons,

  1. Streams passed to that method may be used in some other place. Reusable streams are available in java. If the stream is closed it cannot be reopened and reused.
  2. In case of Exception when closing the stream you don't know how to handle that. Because you are dealing with general inputstream and it may come from any place like File, Network etc.

The class opens the stream is responsible for closing it.

Vijayakumar
  • 303
  • 4
  • 10
0

I don't think that the JVM spec makes any guarantee about that. You really are supposed to finally close these resources.

When the process ends, the operating system will release all resources associated to it (including memory, file handles, and network sockets).

There are OS facilities to check about open files and streams

abhinavsinghvirsen
  • 1,853
  • 16
  • 25
0

No you don't have to do it because it may be used somewhere further in the code.

Mitchy
  • 1
  • 1
0

You do document the method with: "Closes the stream" and change the name method to like readAndClose.

Or create a parameter boolean closeStream and close if true.

Also if the stream doesnt support mark/seek/reset there's no reason to keep it open.

Marcos Vasconcelos
  • 18,136
  • 30
  • 106
  • 167
  • Document the method with *close**s** the stream*. Otherwise, it looks like an instruction to you, the caller. – Andy Turner Oct 26 '17 at 12:20
  • 1
    "create a parameter boolean closeStream and close if true." Better not to do that, because the meaning of that parameter is ambiguous: you might get it confused with some other method, and think that "true" means "keep the stream open". If you're going to use a parameter, a two-element enum `enum Blah { KEEP_OPEN, CLOSE }` is better because it self-documents the meaning. – Andy Turner Oct 26 '17 at 12:23
  • In any case that it could closes the stream you need to doc it – Marcos Vasconcelos Oct 26 '17 at 12:23
  • Right, but there is a big difference between telling somebody to do something, and saying that you will do it. It might seem like nit-picking to quibble over 1 character, but it changes the meaning substantially. – Andy Turner Oct 26 '17 at 12:24
  • 1
    Oh.. thanks pointing it out, and I did like the solution with the enum – Marcos Vasconcelos Oct 26 '17 at 12:28