Thanks for looking into this,
I was out of the codebase for a while, and definitely could write down
something meaningless.
For this particular case, obviously close() inside try {} should be
removed. This is something slipped from my vision when I looked into
this. Luckily, JDBC javadoc says close() on closed statement does
nothing, so likely that's why the tests didn't fail :)
The reason I looked at this is that I have a tool, which shows
possible connection leaks, and shows a sequence that leads into it. I
can confirm, that one these files leak could *possibly* happen.
As for nested try{}, I found this way is ugly, but the only one
semantically correct. I used this way in ThrowOnPartialSchemaStrategy
change in same changeset. The main reason of its necessity is that
close() on any JDBC resources can produce SQLException itself, so all
resources cannot be handled in same finally {}.
I'll reconsider changes in DerbyPkGenerator ASAP (no doubt for me that
the changes were required though).
Thanks,
Andrey
2011/2/20 Andrus Adamchik <[email protected]>:
> I don't think this particular patch is correct. First of all it calls
> "select.close()" twice, second nesting of close operations is suspect
> (select.close() after c.commit()).
>
> On the general semantics of JDBC code. What are are normally using in Cayenne
> (with the exception of a few overlooked cases like this one), is a nested set
> of try { } finally {} over ResultSet, Statement, Connection. This ensures
> that "close" is called in the right sequence and regardless of the exception
> state. Also this seems to work pretty well (no bugs were reported that I can
> attribute to this code).
>
> Andrus
>
> On Feb 16, 2011, at 10:56 AM, [email protected] wrote:
>> Modified:
>> cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/src/main/java/org/apache/cayenne/dba/derby/DerbyPkGenerator.java
>> URL:
>> http://svn.apache.org/viewvc/cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/src/main/java/org/apache/cayenne/dba/derby/DerbyPkGenerator.java?rev=1071175&r1=1071174&r2=1071175&view=diff
>> ==============================================================================
>> ---
>> cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/src/main/java/org/apache/cayenne/dba/derby/DerbyPkGenerator.java
>> (original)
>> +++
>> cayenne/main/trunk/framework/cayenne-jdk1.5-unpublished/src/main/java/org/apache/cayenne/dba/derby/DerbyPkGenerator.java
>> Wed Feb 16 08:56:55 2011
>> @@ -58,9 +58,10 @@ public class DerbyPkGenerator extends Jd
>> }
>>
>> Connection c = node.getDataSource().getConnection();
>> + PreparedStatement select = null;
>>
>> try {
>> - PreparedStatement select = c.prepareStatement(
>> + select = c.prepareStatement(
>> SELECT_QUERY,
>> ResultSet.TYPE_FORWARD_ONLY,
>> ResultSet.CONCUR_UPDATABLE);
>> @@ -91,6 +92,9 @@ public class DerbyPkGenerator extends Jd
>> return nextId;
>> }
>> finally {
>> + if (select != null) {
>> + select.close();
>> + }
>> c.close();
>> }
>> }
>
>
