You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "cortlepp-intershop (via GitHub)" <gi...@apache.org> on 2023/11/14 12:14:53 UTC
[PR] refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591) [commons-dbcp]
cortlepp-intershop opened a new pull request, #322:
URL: https://github.com/apache/commons-dbcp/pull/322
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 taht not other changes are necessary.
This closes [#DBCP-591](https://issues.apache.org/jira/browse/DBCP-591).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591) [commons-dbcp]
Posted by "psteitz (via GitHub)" <gi...@apache.org>.
psteitz commented on PR #322:
URL: https://github.com/apache/commons-dbcp/pull/322#issuecomment-1899290620
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591) [commons-dbcp]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #322:
URL: https://github.com/apache/commons-dbcp/pull/322#issuecomment-1899350079
The build is broken.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591) [commons-dbcp]
Posted by "psteitz (via GitHub)" <gi...@apache.org>.
psteitz commented on PR #322:
URL: https://github.com/apache/commons-dbcp/pull/322#issuecomment-1899372878
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
> <https://github.com/apache/commons-dbcp/pull/322#issuecomment-1899350079>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AALJJ2TTV353K3TOV6W4AGLYPGSCNAVCNFSM6AAAAAA7KWNOPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJZGM2TAMBXHE>
> .
> You are receiving this because you modified the open/close state.Message
> ID: ***@***.***>
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591) [commons-dbcp]
Posted by "cortlepp-intershop (via GitHub)" <gi...@apache.org>.
cortlepp-intershop commented on PR #322:
URL: https://github.com/apache/commons-dbcp/pull/322#issuecomment-1828575790
Hey @garydgregory @psteitz, just checking in: did you have a chance to think about this some more and how we could proceed here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591) [commons-dbcp]
Posted by "cortlepp-intershop (via GitHub)" <gi...@apache.org>.
cortlepp-intershop commented on PR #322:
URL: https://github.com/apache/commons-dbcp/pull/322#issuecomment-1817449233
Hey @garydgregory any thoughts?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591) [commons-dbcp]
Posted by "psteitz (via GitHub)" <gi...@apache.org>.
psteitz merged PR #322:
URL: https://github.com/apache/commons-dbcp/pull/322
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591) [commons-dbcp]
Posted by "cortlepp-intershop (via GitHub)" <gi...@apache.org>.
cortlepp-intershop commented on PR #322:
URL: https://github.com/apache/commons-dbcp/pull/322#issuecomment-1817524088
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](https://issues.apache.org/jira/browse/DBCP-591) 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591) [commons-dbcp]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #322:
URL: https://github.com/apache/commons-dbcp/pull/322#issuecomment-1817529133
Hello @psteitz ?
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] refactor: use ReentrantLock in PoolableConnection.close (#DBCP-591) [commons-dbcp]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #322:
URL: https://github.com/apache/commons-dbcp/pull/322#issuecomment-1817479141
> Hey @garydgregory any thoughts?
Hello @cortlepp-intershop
There is no test to prove this fixes anything.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org