Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[JBJCA-1445] Avoid deadlock when the connection is being destroyed #791

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[JBJCA-1445] Avoid deadlock when the connection is being destroyed #791

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 9 additions & 2 deletions .../core/connectionmanager/pool/mcp/SemaphoreConcurrentLinkedDequeManagedConnectionPool.java
View file
Open in desktop
Expand Up @@ -573,7 +573,14 @@ else if (debug)
{
if (pool.getInternalStatistics().isEnabled())
pool.getInternalStatistics().deltaBlockingFailureCount();

synchronized (cls) {
Copy link
Contributor

@tadamski tadamski Jan 26, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@gazda1983 gazda1983 Jan 26, 2024 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

if (cls.isEmpty()) {
if (debug) {
log.debug("Timeout trying acquire lock but cls is empty. Releasing the orphaned lock");
}
pool.getLock().release();
}
}
// We timed out
throw new ResourceException(
bundle.noMManagedConnectionsAvailableWithinConfiguredBlockingTimeout(
Expand Down Expand Up @@ -746,7 +753,7 @@ else if (debug)
cl.destroy();
}

if (releasePermit)
if (releasePermit || (clw != null && clw.hasPermit()))
{
pool.getLock().release();
}
Expand Down