refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591)

[cortlepp]

This PR removes synchronized from PoolableConnection.close() and replaces it by a ReentrantLock which is owned by the connection. I could not find any references in the code which externally synchronize on this connection, so I think that not other changes are necessary.

This closes #DBCP-591.

[cortlepp]

Hey @garydgregory any thoughts?

[garydgregory]

Hey @garydgregory any thoughts?

Hello @cortlepp-intershop
There is no test to prove this fixes anything.

[cortlepp]

True, but do you have a suggestion on how to write such a test? Since the Issue is not that something doesn't work, it's rather a performance thing. The current implementation, because it uses synchronized and then does some I/O, doesn't work "gracefully" with the new VirtualThreads introduced in JDK 21 in the sense that the JVM cannot unmount the VirtualThread during this I/O operation. This doesn't break anything in a functional sense, it just impacts the scheduling (and therefore the throughput) of the application negatively. Using a ReentrantLock fixes this because, unlike synchronized, it does not prevent the JVM from unmounting the VirtualThread during this I/O operation.

In the discussion on the linked issue Phil Steitz also suggested that locking on this method might not be necessary at all, but since he wasn't sure I implemented the "safe" version using the alternative locking mechanism.

In order to test that this works, my only idea would be to start the test runner with the JVM property -Djdk.tracePinnedThreads=full, and the assert that no pinning stacktrace is present in the log. But this seems to me to be very brittle, hard to implement and also somewhat overkill for the situation at hand.

[garydgregory]

Hello @psteitz and all,
Any thoughts on whether we should bring in this PR? How can we make sure that this does make things worse and in fact better on Java 8, 11, 17, and 21? Is there a JMH benchmark that would make sense to write?

[cortlepp]

Hey @garydgregory @psteitz, just checking in: did you have a chance to think about this some more and how we could proceed here?

[psteitz]

Sorry for the very slow response. I think this is good to merge. I agree tests would be nice, but like the OP, I can't think of a simple way to demonstrate the impact of the pinning, which is pretty certain to happen on the (not normal) execution paths where connections get physically closed. This should have no impact on performance in earlier JDKs. I also verified that no other code relies on this object's monitor.

[garydgregory]

The build is broken.

[psteitz]

Crap. Forgot to check checkstyle output. Seems a formatting nit. Will fix in a few hours.

…

On Thu, Jan 18, 2024 at 3:56 PM Gary Gregory ***@***.***> wrote: The build is broken. — Reply to this email directly, view it on GitHub <#322 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALJJ2TTV353K3TOV6W4AGLYPGSCNAVCNFSM6AAAAAA7KWNOPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJZGM2TAMBXHE> . You are receiving this because you modified the open/close state.Message ID: ***@***.***>