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

Issue #24805 Make ConnectionPool code more strict and do not return Enlisted resources #25373

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

escay
Copy link
Contributor

@escay escay commented Feb 14, 2025

Make ConnectionPool code more strict and do not return Enlisted resources.

Throw isBusy and enlisted IllegalArgumentExceptions on purpose in the goal to find out why #24805 sometimes occurs.
This allowed me to see ConnectionPool.internalGetResource skip getResourceFromTransaction logic in some cases, and then calling getUnenlistedResource returned an Enlisted resource.

Therefor adding logic:
Add new isEnlisted logic in the getResourceFromPool method to possibly fix #24805 Remove getMatchedResourceFromPool method. Status is: passes all 209 "./runtests.sh jdbc_all" tests.

Also refactored some old TODO statement: replace getMatchedResourceFromPool implementation with a call to getResourceFromPool.

IllegalArgumentException on purpose in the goal to find out why eclipse-ee4j#24805
sometimes occurs. Add new isEnlisted logic in the getResourceFromPool
method to possibly fix eclipse-ee4j#24805 Remove getMatchedResourceFromPool method.
Status is: passes all 209 "./runtests.sh jdbc_all" tests.
@OndroMih
Copy link
Contributor

OndroMih commented Feb 17, 2025

Running the Jakarta EE TCK here (with master merged): https://ci.eclipse.org/jakartaee-tck/job/10/job/eftl-jakartaeetck-run-100/295/ - 👍 passed
Running the standalone Jakarta EE TCKs: https://ci.eclipse.org/jakartaee-tck/view/EFTL-Certification-Jobs-10/job/eftl-jakartaeetck-run-standalone/163/ - 👍 passed

IllegalArgumentException on purpose in the goal to find out why eclipse-ee4j#24805
sometimes occurs. Add new isEnlisted logic in the getResourceFromPool
method to possibly fix eclipse-ee4j#24805
…ol "do not

return enlisted resource handle logic" is not added in method
getResourceFromPool, while the "IllegalSateExceptions for the enlisted
state" are added at the end of method getResourceFromPool.
@dmatej
Copy link
Contributor

dmatej commented Mar 24, 2025

Is there a reason why is this still in a Draft mode?

@escay
Copy link
Contributor Author

escay commented Mar 24, 2025

@dmatej Just discussed the reason today in #24805 latest comments.
This fix prevents the problem from occurring, as shown in the integration test.
But I wonder if it should be supported, or that the fix should be: throw an exception, or log a warning.
Last month I have been testing without the fix by changing my application code, this now runs stable when I do not expose a Connection from one bean method to another bean method. Question is: should exposing a Connection be allowed? If this should be allowed then I can remove the draft mode. If it should not be allowed I would opt for logging a warning + this fix.

@escay escay marked this pull request as ready for review March 28, 2025 15:24
@escay
Copy link
Contributor Author

escay commented Mar 28, 2025

Changed to ready for review. Assuming now that exposing a Connection from one bean method to another bean method should be possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NPE in ConnectorXAResource.getResourceHandle causes connection pool problems
3 participants