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

Steven de Jong

创建于 2011年10月18日 08:59

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.

Serge Baranov

创建于 2011年10月19日 10:51

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".

Alexander Ashitkin

创建于 2011年10月19日 20:53

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

regards, Alex

Russ Kociuba

创建于 2011年10月19日 21:19

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.

Serge Baranov

创建于 2011年10月19日 21:21

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

ANONYMOUS

创建于 2011年10月20日 01:11

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

Alexander Ashitkin

创建于 2011年10月20日 08:50

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):

regards, Alex

Bas Leijdekkers

创建于 2011年10月20日 20:09

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

Bas Leijdekkers

创建于 2011年10月20日 20:17

It seems you found a bug: http://youtrack.jetbrains.net/issue/IDEA-75774

Bas

Thibaut

创建于 2011年10月24日 10:50

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
}

Permanently deleted user

创建于 2017年01月17日 11:57

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);
    }
}