Support for loom #230
Support for loom #230
Conversation
Codecov Report
@@ Coverage Diff @@
## master #230 +/- ##
=========================================
Coverage ? 83.19%
Complexity ? 787
=========================================
Files ? 42
Lines ? 3076
Branches ? 312
=========================================
Hits ? 2559
Misses ? 410
Partials ? 107
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
@zenghu1chen |
| @@ -33,6 +34,8 @@ | |||
| */ | |||
| public interface PooledObject<T> extends Comparable<PooledObject<T>> { | |||
|
|
|||
| ReentrantLock lock = new ReentrantLock(); | |||
There was a problem hiding this comment.
A global variable? That looks like an anti-pattern. -1 unless you can explain what I am misunderstanding.
There was a problem hiding this comment.
This lock is to replace synchronized(PooledObject) with ReentrantLock in the GenericObjectPool,BaseGenericObjectPool and GenericKeyedObjectPool class.
There was a problem hiding this comment.
Hello @zenghu1chen
A mutable global variable in an interface is an anti-pattern IMO, you've not addressed this issue in your comment.
I'm trying to write test cases to reproduce the deadlock situation. Please wait for a while, thank you. |
|
To help with tracing, could you provide the versions of both Jedis and Commons Pool that you are using? |
Hello @psteitz |
|
@zenghu1chen |
@garydgregory |
|
Hi @zenghu1chen |
|
Many thanks for the test and analysis. I will look at this in the next couple of days too. The test looks interesting as it is doing something that is not expected - invalidating an object after returning it to the pool. Generally, once you return an object to the pool, you should not reference it subsequently as it may have been handed out to another client. To check and invalidate idle instances in the pool, its best to enable testWhileIdle, enable the evictor and let the pool handle it. But there should be protection at the PooledObject level to prevent a deadklock here, so worth investigating. Is the Jedis code doing this? It would be good to open a JIRA for this issue and we should probably separate the deadlock issue (if it turns out there is one) from the use of intrinsic locks. A catchall issue to replace all intrinsic locks might make sense at this point. I agree with Gary on the smelliness of putting the lock in the PooledObject interface. An alternative might be to expose a lock() method and implement it in DefaultPooledObject using a ReentrantLock. Unfortunately, without a default implementation that would be an incompatible change, so not possible in 2.x. Maybe someone else has a clever idea on how to provide a default that would work. |
|
I looked at the test case. I would expect to see the test threads getting a lot of IllegalStateExceptions due to what amounts to a misuse of the API. We should probably clarify that invalidating an object after you have returned it to the pool (or doing anything else with it) violates the pool contract. The borrow - invalidate sequence in the test could be intermediated by a borrow by another thread. That will cause ISE when the other thread returns it if that happens after the invalidate's destroy completes. The invalidates will throw ISE if another thread has destroyed the instance (which the test setup allows). All of this is to say, you can't do what the test is doing. Does the Jedis code do this? |
|
One more comment on this. Your Jedis version also looks very old. You might try updating it along with the pool version. this part of the stack trace above looks odd to me: I downloaded the tag for the version of Jedis that you mentioned and I can't find the sourses for the class above in it (or anywhere else online). What is odd is that the GOP destroy method is private. Whatever the thing above is, it should not be calling that method directly, but instead using invalidateObject. The intrinsic lock on the pooled object makes sense there and I can see the pinning, but I don't see any evidence or cause for deadlock here. |
|
@psteitz Thank you for your reply. |
|
Returning back to this, I think we should eliminate the use of intrinsic locks here. The example is a little strange, but I can imagine other scenarios where pinning happens due to the intrinsic locks on PooledObjects. The code referenced in the PR acquires a lock on a PooledObject'ss monitor. DefaultPooledObject uses syncrhonized methods to guard state changes. We obviously can't add an explicit lock to the PooledObject interface. The simplest way that I can see to solve this is to add lock() and unlock() methods to the PooledObject interface, defaulted to no-ops (with warning labels) and implemented in DefaultPooledObject with a ReentrantLock. Any better ideas? |
|
@psteitz Thank you for your suggestion. I have already resubmitted the code. |
|
@psteitz / @zenghu1chen please take a look on my code review |
src/main/java/org/apache/commons/pool3/impl/BaseGenericObjectPool.java
Outdated
Show resolved
Hide resolved
|
Hello no news for this PR ? I also need this feature, I use apache polls with virtual threads |
There was a problem hiding this comment.
- Please note that Loom will eventually address pinning of
synchronized– see this Java 23 EA announcement https://inside.java/2024/06/22/quality-heads-up/ . The loom variant is located on the Project Loom Early-Access Builds page at https://jdk.java.net/loom/ . - The current PR still lacks unit tests, and it would be a waste of time to wait for new unit tests.
My project uses jedis component with loom, but there are cases of pinning and even deadlocks when redis times out. By replacing synchronized with ReentrantLock in the code, the above situations will be avoided. Below is the stacktrace when pinning occurs.