'OutputStreamWriter' should be opened in front of a 'try' block and closed in the corresponding 'finally' block

Hi, All
Doesn't it look confusing for you? Open in front of try and close in finally?

regards, Alex

11 comments
Comment actions Permalink

I don't really understand what's the issue here. Yes, this is confusing -- but is this how IntelliJ does it when autogenerating code? Please clarify the issue.

0
Comment actions Permalink

I guess the confusing part is "in front of try", which can be interpreted like "before try" which would be incorrect, probably the wording should be "in the beginning of try" or "inside try".

0
Comment actions Permalink

Hi, Sergey.
The point here isn't wording only, but inspection itself also. Please, try to guess, what's wrong here:
io-resource-inspection.png

regards, Alex

0
Comment actions Permalink

If you open the inspection setting there is a check box for "Allow resource to be opened in a try block" which will fix this false positive.

See this thread for more discussion: http://devnet.jetbrains.net/message/5251975#5251975

Also this inspections *was* broken for quite some time (http://youtrack.jetbrains.net/issue/IDEA-19114).  The comment from Bas is where I found out about this option.

0
Comment actions Permalink

Thanks, it makes sense indeed. Forgot about that inspection option.

0
Comment actions Permalink

Looks like a bug, probably a regression of http://youtrack.jetbrains.net/issue/IDEA-26256.
Can you submit a new issue?

---
Original message URL: http://devnet.jetbrains.net/message/5323373#5323373

0
Comment actions Permalink

Hi. Russ.
Thank you. I wonder why that option  doesn't enabled by default.
But i still confused with wordings. Resources shouldn't be opened in a front of a 'try' block after all. Sometimes it's a bug, like these code (not alerted by idea):

resource-not-closed.PNG

regards, Alex

0
Comment actions Permalink

The recommended way is to code it like this:


 
class Sample {
  public static void main(String[] args) throws IOException {
    FileOutputStream fos = new FileOutputStream("filename");
    try {
      // read from the file
    } finally {
      fos.close();
    }
  }
}

See also http://www.javapractices.com/topic/TopicAction.do?Id=25

Bas

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

Hi,

I am seeing the same for 'PreparedStatement'

'PreparedStatement' should be opened in front of a 'try' block and closed in the corresponding 'finally' block.

The real validation/inspection where to check if the PreparedStatement is actually closed in a finally block is not working.

See following example code:

The inspector complains about opening the PreparedStatement in front of a try block.

And if I remove the "cleanup" call from the Finally block there is no warning that the preparedStatement is unclosed.

public Foo selectFoo() {
Foo result;

DBWrapper db = null;
PreparedStatement ps = null;
ResultSet rs = null;

try {
db = new DBWrapper();

String query = "SELECT * FROM FOO";

ps = db.prepareStatement(query);
rs = ps.executeQuery();

if(rs != null && rs.next()) {
result = new Foo(rs.getInt("Id"), rs.getString("Name"));
}

} catch (SQLException e) {
logger.error(e);
} finally {
cleanup(rs, ps, db); //I can remove this call to the method cleanup without getting warning for unclosed JDBC resources
}

return result;
}

public void cleanup(ResultSet rs, Statement ps, DBWrapper db) {
try {
if (rs != null)
rs.close();
} catch (SQLException e) {
logger.error(e);
}
try {
if (ps != null)
ps.close();
} catch (SQLException e) {
logger.error(e);
}
try {
if (db != null)
db.close();
} catch (Exception e) {
logger.error(e);
}
}

 

0

Please sign in to leave a comment.