HHH-18325: Prevent comparison between proxied and non-proxied statements

[mc1arke]

Some JDBC connection pools create a dynamic reflective Proxy wrapper around connections in the pool, plus the generated statements and the result sets those statements create, with the proxies allowing for methods calls to be intercepted by the connection pool for the purposes of returning items to the pool when they're released rather than destroying the underlying database connection. Tomcat's JDBC pool implementation does not correctly wrap the full chain of objects, meaning the raw Statement can be retrieved from a ResultSet rather than returning the proxied statement. This results in the ResourceRegistry cache attempting to store both the proxied Statement and the non-proxied Statement in its cache, but encountering an exception when the HashMap encounters two entries with the same HashCode and then attempts to differentiate them with an equals call which Tomcat's wrapper expects both instances to be proxied connections. To overcome this issue in Tomcat, as well as any other pool implementation which may use Proxy classes but leak the un-proxied entries, the original PreparedStatement used to create the ResultSet is being passed into the GeneratedValuesHelper for passing into the ResourceRegistry, rather than the GeneratedValues helper attempting to extract the Statement from the ResultSet.

---

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license
and can be relicensed under the terms of the LGPL v2.1 license in the future at the maintainers' discretion.
For more information on licensing, please check here.

---

https://hibernate.atlassian.net/browse/HHH-18325

[gavinking]

Tomcat's JDBC pool implementation does not correctly wrap the full chain of objects, meaning the raw Statement can be retrieved from a ResultSet rather than returning the proxied statement.

That sounds like a bug in the Tomcat connection pool, and is something that should be fixed in Tomcat.

Thus, I would argue against merging this PR.

That said, I am not at all a fan of our ResourceRegistry and:

1. I would much prefer to see it go away,
2. but, if that's impossible, could it at least be changed to use an IdentityHashMap?

@sebersole ?

[mc1arke]

That sounds like a bug in the Tomcat connection pool, and is something that should be fixed in Tomcat.

Agreed, the original detection of this is due to an issue in Tomcat, which I've raised a separate bug and proposed fix for

Thus, I would argue against merging this PR.

The bug in Tomcat has highlighted that Hibernate is discarding objects that it then has to retrieve later on but could be easily passed through the calls to the ResourceRegistry. If the ResourceRegistry can be killed off then this largely becomes irrelevant, but just now Hibernate is presuming that statement.equals(statement.getResultSet().getStatement()) == true which isn't mentioned as a requirement in the JDBC specification, so this MR still provides value in making Hibernate resilient to different Driver or Connection Pool implementations.

but, if that's impossible, could it at least be changed to use an IdentityHashMap?

There's nothing in the JDBC spec that says the same instance has to be returned for a Statement and the calls to a child ResultSet's getStatement method (i.e. Connection Pools or Drivers could create new wrapper/proxy instances for those 2 calls) so an IdentityHashMap still carries a risk of a memory leak.

[sebersole]

That said, I am not at all a fan of our ResourceRegistry and:

1. I would much prefer to see it go away,
2. but, if that's impossible, could it at least be changed to use an IdentityHashMap?

IdentityHashMap is not going to help, so your (2) is a moot point.

As for ResourceRegistry going away, I'm not against it but it does serve a few purposes which then need to be handled in some other way. I think we have to look at the different parts of the ResourceRegistry:

1. LOB reference tracking. As we create LOB references via the JDBC Connection, we register them here to ensure we can clean them up later.
2. Statement/ResultSet tracking. In many ways this is used as a super complicated try-with-resource and in many cases we can probably just not do that anymore. However, there are cases (scrolling, ..) where we do need to track these and we'd need to come up a different solution.

Historically that second usage has always been in Hibernate, it was just previously done in a worse way before the introduction of LogicalConnection and the redesign of batching. I've never liked the fact that we keep this, but dropping it is not easy (or at least was not when I last tried).

[sebersole]

I'm fine with the change. I agree with @mc1arke that it, at worst, makes Hibernate more robust.

[gavinking]

I'm fine with the change. I agree with @mc1arke that it, at worst, makes Hibernate more robust.

OK, fine. I withdraw my objection.

[dreab8]

Hi @mc1arke ,

thanks for your PR, Aplogize for taking so long to come back to you but we have been really busy.
In the meantime most of the changes of this PR are been fixed.

I have rebased your PR and created #11048.