Fix borrowObject timeout wrapping#3666
Conversation
Override borrowObject(long borrowMaxWaitMillis) in GenericObjectPool to apply connection wrapping when borrowing with timeout. This ensures connections borrowed with timeout are properly returned to the pool when closed via try-with-resources. Without this fix, borrowObject(timeout) returns unwrapped connections that don't auto-return to pool, causing connection leaks.
|
Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset. In case there are security findings, they will be communicated to you as a comment inside the PR. Hope you’ll enjoy using Jit. Questions? Comments? Want to learn more? Get in touch with us. |
a-TODO-rov
left a comment
There was a problem hiding this comment.
Hey, @JiHongKim98
Thanks for the help !
I see you've overridden borrowObject(long borrowMaxWaitMillis) , but didn't override borrowObject(final Duration borrowMaxWaitDuration) , which is currently the officially supported method. So for this i will put PR in request changes.
On a side note - did you think what will happen if the commons library introduce another overload of this method - borrowObject(final Instant borrowMax) for example. Probably we will still fail silently. So do you think there is some more general solution ?
I completely realize that adding another overload is not highly probable, but i would like to hear your thoughts on this.
|
@a-TODO-rov Thank you for the review! You're right - I'll add the Duration overload. About your question: I thought about using dynamic proxy to handle future overloads automatically. But I think that would add overhead and make the code more complex. For now, I think the best approach is to explicitly override each method that commons-pool provides. If you're concerned about silent failures when new overloads are added, we could also add a defensive test that validates all borrowObject methods are properly overridden using reflection. What do you think about these approaches? Thanks again for the thorough review! |
Override borrowObject(Duration) in GenericObjectPool to apply connection wrapping, ensuring connections borrowed with Duration timeout are properly returned to pool when closed via try-with-resources. Add integration test following the same pattern as borrowObject(long) test.
Rename borrowObject test methods to be more explicit: - genericPoolShouldWorkWithBorrowObjectTimeout -> TimeoutMillis - genericPoolShouldWorkWithBorrowObjectDuration -> TimeoutDuration This makes it clearer which parameter type each test covers.
|
Thanks @JiHongKim98 |
fix #3623
Override borrowObject(long borrowMaxWaitMillis) in GenericObjectPool to apply connection wrapping when borrowing with timeout. This ensures connections borrowed with timeout are properly returned to the pool when closed via try-with-resources.
Without this fix, borrowObject(timeout) returns unwrapped connections that don't auto-return to pool, causing connection leaks.
Make sure that:
mvn formatter:formattarget. Don’t submit any formatting related changes.