Correctly Proxy Statement returned from ResultSet #742
+382
−8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The tomcat-jdbc connection pool creates reflective proxies around Connections, Statements, and ResultSets to allow interception of 'close' calls, and consistency of traversing the link back from a child element to its parent - e.g. Statement to Connection. However, the link from ResultSet to Statement does not intercept the call so a non-proxied Statement is returned, as well as the `equals` method not handling the comparison against a non-proxied class correctly. To overcome this, an invocation handler is being created for wrapping a ResultSet returned from any of the relevant Statement methods, with that invocation handler intercepting the `getStatement` call and returning the proxied creating statement. The `equals` checks have also been altered to handle non-proxied instances being passed in to them.
Contributor
|
Thanks for the PR. All looks reasonable to me. There are a few bits of potential clean-up I noticed both in the patch and the surrounding code. I'll do that after I've merged this PR. |
|
Unfortunately this caused some issues. Where we previously relied on catching |
Member
Author
|
@chrisw-s Do you have a stacktrace that shows the exception you're getting or steps to reproduce the issue? |
|
Just this: This is with JTDS. It works fine in 10.1.26 |
|
Hello, Same is observed on my side with the following strack trace below. With latest tomcat 9.091 is ok while after updating to tomcat 9.092/93 it fails. The problem is that DB initiation is delegated to liquibase and after that fails application initialisation. |
Member
Author
|
Ah, ok. It's an exception being thrown whilst executing the underlying statement, which is happening outside the existing try/catch block that unwraps the exception. I'll raise a Bugzilla record and propose a PR to fix it. |
Member
Author
|
Proposed fix in #744 |
|
Hello, I think this change introduced an important performance penalty comparing my tests using 10.1.26 vs 10.1.27 10.1.27: 10.1.26: Call is direct in 10.1.26 and I guess that is the root cause of the bug, but instead of using reflection we can just create a wrapper calling the real delegate |
Contributor
|
You should test the latest version (although I suspect you'll see the same results). Use of proxies rather than wrappers was a design decision for jdbc-pool so it could handle changes in the JDBC API. |
|
Hello proxies could be implemented dynamically using one of ASM libraries; Reflection is very expensive and taking into consideration the huge number of applications using Tomcat this change will generate a lot of CO2. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The tomcat-jdbc connection pool creates reflective proxies around Connections, Statements, and ResultSets to allow interception of 'close' calls, and consistency of traversing the link back from a child element to its parent - e.g. Statement to Connection. However, the link from ResultSet to Statement does not intercept the call so a non-proxied Statement is returned, as well as the
equalsmethod not handling the comparison against a non-proxied class correctly. To overcome this, an invocation handler is being created for wrapping a ResultSet returned from any of the relevant Statement methods, with that invocation handler intercepting thegetStatementcall and returning the proxied creating statement. Theequalschecks have also been altered to handle non-proxied instances being passed in to them.https://bz.apache.org/bugzilla/show_bug.cgi?id=69206