Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Addresses a shortcoming of the Universal Connection Pool pool creation logic in certain test scenarios #8642

Merged
merged 1 commit into from Apr 9, 2024

Conversation

ljnelson
Copy link
Member

@ljnelson ljnelson commented Apr 9, 2024

This PR is quite specialized.

This PR addresses a shortcoming in the Universal Connection Pool API when named connection pools are involved.

In the UCP API, when you call poolDataSource.getConnection(), a backing UniversalConnectionPool is created just-in-time, configured indirectly by a subset of configuration present on the PoolDataSource object. One part of that configuration is a Java Beans property called connectionPoolName.

Apparently UniversalConnectionPool instances are stored by name in a UniversalConnectionPoolManager singleton.

If you do not call setConnectionPoolName(String) on a PoolDataSource, a (hopefully guaranteed to be truly?) random name for the connection pool is generated internally by UCP, and your PoolDataSource's setConnectionPoolName(String) method is called with the new random name (none of this is documented, but unit tests show it to be true). In versions of Helidon prior to 4.0.6 this could result, in unit test scenarios, in dozens if not hundreds of UniversalConnectionPool instances all backing what is for all intents and purposes the same data source.

Helidon 4.0.6 tried to help this situation by checking to see if the connectionPoolName property was not set in configuration. If it was not, then the name defaulted to the data source name (so if you had a data source named fred, its backing connection pool would also be named fred, and you'd see that information in UCP logs and so on).

The problem is that it turns out there is no way to associate a PoolDataSource with a given connectionPoolName with an already-existing UniversalConnectionPool instance that has the same name (!). That is, when poolDataSource.getConnection() is called, a blind attempt at creating the backing connection pool is attempted (and fails), even if such a pool already exists, rather than an association operation. This scenario can crop up in certain user unit tests, apparently.

(It is a corollary of this that a user using only the PoolDataSource API, as instructed by the UCP documentation, has no way of knowing in advance whether the name she has selected for the connectionPoolName property is going to result in a SQLException or not.)

Prior to this change, the user has a couple of choices:

  • destroy all connection pools "by hand" in her unit tests. This is probably the proper thing to do, but it is tedious.
  • Proactively ensure that whatever connectionPoolName she selects for a given unit test is unique in all of her tests, resulting in possibly dozens if not hundreds of UniversalConnectionPool instances in her test suite (see above).

Neither option is particularly friendly.

This PR therefore amends the Helidon 4.0.7 behavior as follows:

  • If a PoolDataSource configuration contains a value for connectionPoolName, no further magic behavior results. If that pool name happens to exist, tough luck. The UCP will fail in whatever way it fails, good or bad, right or wrong.
  • If a PoolDataSource configuration does not contain a value for connectionPoolName:
    • the UCPBackedDataSourceExtension will now check to see if the data source name can be used as a pool name. This will only be the case if an equivalently-named connection pool does not already exist.
    • If such a named connection pool does not already exist, the extension will set the connectionPoolName property to the value of the data source name (as is currently done in 4.0.7).
    • If such a named connection pool does already exist, such as might happen in a unit test where the user has chosen not to destroy pools after each test for whatever reason, the extension will revert to 4.0.5 behavior and will not supply a name for the connectionPoolName property. This will result, as in 4.0.5, in at least two connection pools backing "the same" data source: the pre-existing named one, and the new one with a randomly generated name.

This description is approximately 35x longer than the actual PR so have a look.

…n logic in certain test scenarios

Signed-off-by: Laird Nelson <laird.nelson@oracle.com>
@ljnelson ljnelson added enhancement New feature or request MP P4 jpa/jta java Pull requests that update Java code 4.x Version 4.x labels Apr 9, 2024
@ljnelson ljnelson self-assigned this Apr 9, 2024
@oracle-contributor-agreement oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Apr 9, 2024
@ljnelson ljnelson added this to Sprint Scope in Backlog Apr 9, 2024
Copy link
Member

@arjav-desai arjav-desai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ljnelson ljnelson merged commit 7b11d60 into helidon-io:main Apr 9, 2024
12 checks passed
Backlog automation moved this from Sprint Scope to Closed Apr 9, 2024
@ljnelson ljnelson added this to the 4.0.8 milestone Apr 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.x Version 4.x enhancement New feature or request java Pull requests that update Java code jpa/jta MP OCA Verified All contributors have signed the Oracle Contributor Agreement. P4
Projects
Backlog
  
Closed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants