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

Fixes issue #24849 make relevant methods synchronized in LocalTxConnectionEventListener #24851

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

escay
Copy link
Contributor

@escay escay commented Mar 11, 2024

Make relevant methods synchronized in LocalTxConnectionEventListener and protect associatedHandles from external clear calls.

I first tried to move most of the ConnectorXAResource.resetAssociation method inside a synchronous method in the LocalTxConnectionEventListener, but that also meant two other methods from ConnectorXAResource needed to be refactored and moved into LocalTxConnectionEventListener. So I undid that and chose for cloning the map and returning a clone to make the method getAssociatedHandles thread safe. This is a different solution as used in the Payara fix I mention in the issue. But I think it is better to leave the association and disassociating of ManagedConnection logic inside the ConnectorXAResource code.

I did not add multi threading unit tests, since only the basic synchronized(this) approach is used to protect the associatedHandles map. No need to test java. I did add some basic unit test logic for the add and remove logic used by ConnectorXAResource.

…LocalTxConnectionEventListener and protect associatedHandles from external clear calls.

Fixes issue eclipse-ee4j#24849 make relevant methods synchronized in LocalTxConnectionEventListener and protect associatedHandles from external clear calls.
LocalTxConnectionEventListener and protect associatedHandles from
external clear calls. Process review comment and fix typo.
@pzygielo pzygielo linked an issue Mar 11, 2024 that may be closed by this pull request
@dmatej dmatej added this to the 7.0.14 milestone Mar 11, 2024
@dmatej dmatej added the bug Something isn't working label Mar 11, 2024
@avpinchuk
Copy link
Contributor

Also, because synchronized is only used to guard access to the associated handles map, maybe it's better to use ReentrantReadWriteLock instead?

@escay
Copy link
Contributor Author

escay commented Mar 12, 2024

Also, because synchronized is only used to guard access to the associated handles map, maybe it's better to use ReentrantReadWriteLock instead?

I cannot oversee the impact of using ReentrantReadWriteLock on only the associatedHandles. Some code like the
poolManager.resourceClosed(handle); calls could also be affected by the synchronized approach. I have no easy way to test this, I know the synchronized version works for me for the last years in Payara: payara/Payara@c09e8aa to avoid the exception, so I tried to stay near that solution.

To know for sure I think I need to have found the real cause of issue #24805 and probably also of issue #23483 which I think could be related and run my test set many times.
I am also looking at "dataStructure" get and remove logic in:

https://github.com/eclipse-ee4j/glassfish/blob/3e5203c56e63abf68cece6ddc76cf9ffd91c3d69/appserver/connectors/connectors-runtime/src/main/java/com/sun/enterprise/resource/pool/ConnectionPool.java#L679C1-L682C66

where I suspect resourceHandles can get mixed up, and could perhaps cause this issue as side effect.

LocalTxConnectionEventListener and protect associatedHandles from
external clear calls. Process review comment: use getOrDefault.
@dmatej dmatej merged commit 487af1d into eclipse-ee4j:master Mar 13, 2024
2 checks passed
@escay escay deleted the issue_24849 branch March 21, 2024 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Thread safeness of LocalTxConnectionEventListener
3 participants