|
Patch 1a ready for review.
The patch looks fine to me. Two small questions:
Does ResultSet.creatingStatement have to be volatile? It seems like it's always set before the ResultSet is returned to the user, and not modified later, so I can't see that there are any threading/visibility issues even if it's not volatile. Have you considered this alternative approach: - have a field called owner in am.Statement - in LogicalStatementEntity's constructor, cast physicalPS to am.PreparedStatement and set the owner - implement am.ResultSet.getStatement() as "if (statement_.owner != null) return statement_.owner; else return statement_;" I may have overlooked something, but it sounds like this requires even fewer changes, and, more importantly, it's more robust as it will work automatically if more methods that return ResultSets are added to the JDBC interfaces later. Thanks for looking at the patch Knut Anders.
I think you are correct about the volatile, I will remove it if we end with that approach. I like the alternative approach, but I couldn't implement it exactly as you suggest. Have I misunderstood? LogicalStatementEntity does not implement java.sql.Statement, and can't be set as the owner. A solution would be to move the setting of the ownership into the subclasses, which would currently also only need to happen once due to the class hierarchy (in LogicalPreparedStatement). The other thing that changes, is that the physical statement now has a reference to the logical statement. This could stop the most recently used logical statement from being garbage collected, which is not a big deal I believe. However, I'm wondering if there are other scenarios where this reference could cause side effects / problems? I added a reset of the owner to LogicalStatementEntity.close. I also moved the tests to lang.ResultSetsFromPreparedStatementTest, to have the code tested for other JVM versions than Java SE 6 as well. Have a look at patch (2a) and comment / change it if you want to. I think I like approach 2 better. Patch 2a is exactly what I was thinking, except I assumed LogicalStatementEntity that was implementing Statement. Since all sub-classes of LogicalStatementEntity are implementing Statement, and LogicalStatementEntity is an abstract class (actually, it's not declared as abstract, but in reality it is used as an abstract class I think), either of these approaches should work and make the code even simpler:
a) Declare LogicalStatementEntity as an abstract class than implements java.sql.Statement, and move calls to setOwner() from the sub-classes' constructors into LSE's constructor. b) Remove calls to setOwner() from the sub-classes, and do this in LSE's constructor: physicalPS.setOwner((java.sql.Statement) this); I think I prefer (a) since these two approaches are essentially identical, only (a) makes the assumptions verifiable at compile time. Attached 'derby-3446-2b_rs_getstatement_alternative.diff'.
a) Actually, I had LogicalStatementEntity as an abstract class, until a tool told me it shouldn't be because it didn't have any abstract methods. So I removed the abstract keyword and made the constructor protected instead... I didn't let LSE implement java.sql.Statement, because I saw no need for it (until now). It does make sense with the current plan, and I have implemented approach a in patch 2b. Thanks for the input :) I'm also wondering if it would make sense to refer to the prepared statements as am.PreparedStatement, since casts are starting to pop up in the code. Is the code a little too generic? This would be handled in another issue though, and it needs to investigated. Regarding gc, we don't do any clean-up in the logical statement's finalizer, and the only resource the logical statement holds on to is the physical statement. This means that the reference from the physical statement which could keep the logical statement alive a while longer, only affects the lifetime of the physical statement. So it's kind of a self reference, and once all other references to the physical statement are gone, it can be gc'ed (except when the logical statement is still open and referenced, but that's not changed with this patch). If it turns out to be a problem, it can probably be fixed easily with a weak reference.
If the user remembers to close the logical statement there will definitely not be any problem at all, since you reset the reference in LSE.close(). +1 to 2b
I think you are right that it would make sense to refer to the PreparedStatements as am.PreparedStatement. This cache is probably going to be tied to the client driver anyway, and in the client driver you don't get more generic than the am ('Abstract Machine') package... And as long as you need to cast to am.PreparedStatement, it's probably better to declare them as am.PreparedStatement (if nothing else, avoiding the casts gives better compile-time error checking). Attached 'derby-3446-2c_rs_getstatement_alternative.diff', which is the same as 2b except the tests have been removed. I don't think the tests are in the correct location, as they will only be executed for the embedded driver with the current patch. I will a also look into enabling the tests for both CPDS and XADS.
I will see if there is a better place for the tests. Committed patch 2c to trunk with revision 630784. 'derby-3446-3a-stmtpool_test.diff' adds the two tests to jdbcapi.StatementPoolingTest. The case of non-statement pooling environments is already tested in several other tests.
I also tried running the two new tests in a XA environment, and they passed. Patch ready for review. Committed patch 3a to trunk with revision 673327.
Backported to 10.4 with revision 673329. Updated fix versions and fixed typo in the summary. The addition of the test concludes the work on this issue. Fix implemented, test runs cleanly.
Closing. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 |
|
Task
Closed
Minor
Make ResultSet.getStatement return the correct statement when created by a logical statement
I think there are alternative approaches, but when I tried one of them it caused massive code changes to classes in both am and net. I observe that the issue is handled in a similar way as in the patch in the embedded driver.
If you want to run the test with and without the implementing code, remember to also apply the patch for
DERBY-3329.