I/O resource opened but not safely closed inspection misbehaving

Hi all,

Has anyone else noticed this inspection reporting false positives lately? I'm using IntelliJ 8.1.4, and I've attached a screenshot of a case that is pretty clearly wrong. Is this a bug or something in my configuration?

Thanks,
Colin

Picture 7.png

12 comments
Comment actions Permalink

It's not just you:  http://youtrack.jetbrains.net/issue/IDEA-19114

It's been broken for a while now.

0
Comment actions Permalink

Thanks, commented and voted.

Cheers,
Colin

0
Comment actions Permalink

OTOH,

in your snippet, if you open the file BEFORE the try then you get rid of the if(stream != null) if your finally too, without any loss in the safety.

eg :

FileInputStream stream = new FileInputStream(schema);
try {
// use stream
} finally {
      try { stream.close(); } catch (IOException e) { /* LOG E but don't throw it as you'll hide the original exception or return value */ }
}

0
Comment actions Permalink

Is that always true? I guess it makes sense - the FileInputStream constructor throws an exception, but I suppose it never creates the stream in that case.

Thanks, you learn something every day!

Cheers,
Colin

0
Comment actions Permalink

I've always wrtten code as you have too.  I never thought to try what Thibaut suggested since, like you, I wasn't sure if the constructor throwing an exception guarunteed that no streams were opened.

However that solution doesn't work if you want to catch and deal with the FNFE and therefore the inspection bug crops up again.  Also there are methods such as Class#getResourceAsStream() that will return null rather than throwing an exception if the stream can't be opened (although the inspection doesn't report on streams opened in that manner).

This family of inspections can also report on JDBC, Hibernate, socket and a few other types of resources and I'm not sure the alternative solution would work properly for any of those.

In any case - the bug is annoying :-)

0
Comment actions Permalink

yes,  it is always true.

If a constructor throws an exception then you have no way to recover the instance, so the variable will be null.

However do be careful about the finally block too, having an exception thrown from it may make things painful

0
Comment actions Permalink

I don't completely agree with your FNFE case.

Actually you should have 2 try blocks : 1 try/catch for the FNFE case, and 1 try/finally for the IOException as the handling of both cases is likely to be pretty different aren't you ? Also, in case of FNFE, there's nothing to close obviously, so no use for a finally

0
Comment actions Permalink

Why not one try block with 2 catch clasues?  Different styles of coding - neither is right or wrong IMO.

Regardless the inspection is not working as advertised which is the main point.

0
Comment actions Permalink

There's no doubt that the inspection needs to be fixed. My case is overly conservative but the code is basically correct (*), and the inspection used to work correctly in this case.

Cheers,
Colin

(*) However Thibaut is right that my original code didn't correctly handle the case where close() throws an exception. In that case, the IOException from close() is thrown and completely hides the original IOException - in that case you have no way of knowing what originally went wrong. Good catch, that man.

0
Comment actions Permalink

Colin, what might be even more troubling, is that your finally may let go of an exception even though the try didn't (this applies if you have a return in your try, so not in your case)

0
Comment actions Permalink

I don't see why people keep complaining about this : it a constructor ever lets go an exception the variable will never be initialized so there's nothing you can do in a finally block to close the stream/writer as you have no way to reach the (potentially partially created, if it's a lower subclass constructor that lets go of the exception).


thus, from my point of view the "correct" way would be

try {     Writer w = new FileWriter("some path");     try {         // work with writer         w.append("something");     } // any catch if needed related to the // work with writer block     finally {         try {             w.close();         } catch (Throwable t) {             // do not let go of this exception, as we're in finally block, thus will hide any exception thrown in the try         }     } } catch (IOException e) {     // do whatever related to the failure in created the writer/stream, but in any case, the variable will be null }

0
Comment actions Permalink

duplicate post

0

Please sign in to leave a comment.