[JBJCA-1445] Avoid deadlock when the connection is being destroyed #791
Conversation
There was a problem hiding this comment.
@gazda1983 thanks for the PR!
Could you pleaese check what is the file formatting that your are using? I would like to avoid merging those formatting changes as part of this PR.
… one element pool.
|
@tadamski I've restored original formatting. |
|
@gazda1983 thanks! |
| @@ -573,7 +573,14 @@ else if (debug) | |||
| { | |||
| if (pool.getInternalStatistics().isEnabled()) | |||
| pool.getInternalStatistics().deltaBlockingFailureCount(); | |||
|
|
|||
| synchronized (cls) { | |||
There was a problem hiding this comment.
I have a doubt here: iiuc if we still have some connections in the cls map but the race condition occurrs this if will not execute and leak would happen anyway.
There was a problem hiding this comment.
I agree that is not perfect solution, ideally we would have to have synchronized returnConnection and getConnection but that would be performance issue. When there are more connections in the cls the probability of the issue is much lower and the consequences are smaller. In our case (pool with size 1) the returned connection is always the same which is possibly being retrieved from the pool at the same time. Under such circumstances it happens I would say 1 on 10 activemq restarts. So if we have standard configuratiion for example 20 connections in the pool the issue is unlikelly to happen because connections would be rather different. Event if it would happen the consequence is just one less permit to be acquired. In our case it means zero permits and we cannot connect at all.
Of course I would really appraciate any other solution that would solved the issue.
There was a problem hiding this comment.
What's more I cannot just release the lock in case of timeout, because I don't know if the lock is properly acquired, do I ? In case of cls.size = 0 I am sure the lock is invalid because there is no connection which should keep the lock.
There was a problem hiding this comment.
@gazda1983 tbh, I would prefer to create a general solution for this issue, even if it implies some performance penalty. I have opened #792 which adds a lock for idle connection removal. Would you be able to check if it helps for your case? Alternatively, can you share a reproducer so that I can check it myself?
When we have one element pool it is higly possible that connection which is returned to the pool (for example by activemq ExceptionLister) and is going to be destroyed is also being returned at the same time by a different thread. In that case we have race conditions which leads to deadlock on pool lock. We have lock acquired but no connections in the pool.
To avoid this I added addtional checking of hasPermit flag in return connection, to have most recent value in case the flag has been just set in another thread.
Additionally I also added releasing the lock when we have a timeout acquiring the lock and there is no connection in the pool. In that case there is nothing which could release lock.
Some more details in comment by Tomasz Koziel in JBJCA-1445