#786 OS_OPEN_STREAM reported on BufferedReader

open-later
3
2016-04-08
2009-07-29
Jesse Glick
No

On code such as

import java.io.BufferedReader;
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
public class FBTest {
public static void main(String[] args) throws IOException {
InputStream is = new FileInputStream(...);
try {
BufferedReader r = new BufferedReader(new InputStreamReader(is, "UTF-8"));
String line;
while ((line = r.readLine()) != null) {
// whatever
}
} finally {
is.close();
}
}
}

I am getting OS_OPEN_STREAM on the line declaring the BufferedReader using FB 1.3.7 via Ant (*). But this makes no sense: BufferedReader and InputStreamReader are both wrapper streams that do not need to themselves be closed in order to release system resources. Only primitive streams such as FileInputStream, or InputStream's of unknown impl type gotten from a method such as URLConnection.openStream(), must be closed. (**)

(*) I am unable to reproduce this with standalone GUI FindBugs 1.3.8 on a trivial project - but I do not get any warning even if I delete is.close(), so I suspect some misconfiguration with my filters is preventing me from reproducing. I am able to see a "dead assignment" warning if I delete everything after the line declaring 'is' but nothing more, whereas I would expect OS_O_S in this case.

(**) In the case of PrintWriter it is usually necessary to call flush(), but there is no need to close() it if the underlying stream has been closed, and omitted flush()'s often manifest themselves quite predictably as zero-length files which you would catch during elementary testing.

Discussion

  • William Pugh

    William Pugh - 2009-07-29
    • priority: 5 --> 3
    • assigned_to: nobody --> wpugh
    • status: open --> open-later
     
  • William Pugh

    William Pugh - 2009-07-29

    The problem isn't reported in FindBugs 1.3.8 because as of 1.3.8, we don't report unclosed readers or input streams in main methods.

    However, if renamed to something else, FindBugs still reports the problem. The issue is that when the input stream is used to construct the input stream reader and the buffered reader, the analysis looks for the buffered reader to be closed, and is what is usually closed in most of the code bases we look at. If the bufferedReader is closed, the close will be passed down to each decorated reader and input stream, closing the FileInputStream.

    The code you have provided does work, and I'm sorry you are getting a false positive. Is there some particularly reason why you close the input stream rather than the bufferedReader?

     
  • Jesse Glick

    Jesse Glick - 2009-07-29

    As a matter of style, whenever writing code which obtains a stream which might possibly hold on to native resources, I make sure to call close() on it in a finally block which begins immediately after the method (or constructor) call which obtained the stream. This is easy to remember and ensures that you do not do something dumb like

    InputStream is = ...;
    int count = someOtherCalculation(); // oops! might throw some unchecked exception and leave stream open
    try {...}
    finally {is.close();}

    Now the alternative you seem to be suggesting is

    InputStream is = ...;
    BufferedReader r = new BufferedReader(new InputStreamReader(is, ...));
    try {...}
    finally {r.close();}

    which is OK so long as you trust that neither the InputStreamReader nor BufferedReader constructors will ever throw an unexpected exception (even e.g. due to an unsupported encoding, ...). The more paranoid

    InputStream is = ...;
    try {
    BufferedReader r = new BufferedReader(new InputStreamReader(is, ...));
    try {...}
    finally {r.close();}
    } finally {is.close();}

    is as safe as my original version but will also make FB happy. But would prefer to just get no warning on the original code, which is guaranteed to close the underlying stream and is more concise. As I mentioned in the initial comment, closing the wrapper stream _can_ be useful in certain cases - according to the semantics of the wrapper stream - but calling flush() is more likely what you cared about (and close() does not necessarily flush, either):

    OutputStream os = new FileOutputStream(...);
    try {
    PrintStream ps = new PrintStream(os);
    ps.println("hello");
    ps.flush(); // takes care of buffering
    // ps.close() not a substitute for flush() here, and os will be closed anyway
    } finally {
    os.close(); // make sure this is called if FOS constructor returned normally
    }

    I'm not quite sure what to recommend since there seem to be many different styles of closing streams (and no language support to standardize it, though I heard of a proposal for JDK 7 to do so). For example, a lot of people use

    InputStream is = null;
    OutputStream os = null;
    try {
    is = ...;
    os = ...;
    copy(is, os);
    } finally {
    if (is != null) is.close();
    if (os != null) os.close();
    }

    which is not quite right (if is.close() throws IOException then os.close() might never be called); I would use

    InputStream is = ...;
    try {
    OutputStream os = ...;
    try {
    copy(is, os);
    } finally {
    os.close();
    }
    } finally {
    is.close();
    }

    which is clearly safer though there is the extra level of braces nesting.

     
  • Comment has been marked as spam. 
    Undo

    You can see all pending comments posted by this user  here

    Anonymous - 2012-12-29

    I've come across this pattern/style in many code bases as well, where the underlying stream is closed directly, rather than the wrapping BufferedReader.

    This would be one false positive that would be nice to get rid of.

     
    Last edit: Anonymous 2014-08-23
  • Konstantin Komissarchik

    Another vote to please address this false positive. On my particular project, by policy, underlying input streams must be held and closed directly, not via their wrappers. This makes this rule consistently fail for us, forcing it to need to be added to the list of exclusions as useless.

     
  • Jesse Glick

    Jesse Glick - 2016-04-08

    Less relevant in Java 7 since you can use the try-with-resources construction, which is concise, safe, and apparently accepted by FindBugs.

     

Log in to post a comment.

Get latest updates about Open Source Projects, Conferences and News.

Sign up for the SourceForge newsletter:

JavaScript is required for this form.





No, thanks