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.

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

Agroal does not provide existing connections from the pool when new connections cannot be established and pool<MIN_SIZE #31246

Closed
sschu opened this issue Feb 17, 2023 · 9 comments · Fixed by #31762
Labels
area/agroal kind/bug Something isn't working
Milestone

Comments

@sschu
Copy link
Contributor

sschu commented Feb 17, 2023

Describe the bug

  • We are running Keycloak using Quarkus on MS Azure with MS Azure SQL as database
  • We are regularly experiencing connection problems to the Azure DB where some connections drop and new connections cannot be established but existing connections still work
  • With Keycloak based on Wildly/IronJacamar as pooling implementation, there were request processing errors - with Quarkus/Agroal-based Keycloak 20 requests cannot be processed anymore
  • note this is a copy of https://issues.redhat.com/browse/AG-201

Expected behavior

GIVEN there are database connection problems
AND the current number of connections is below MIN_SIZE
AND there are still valid connections in the pool
AND new connections cannot be created
WHEN a connection is requested from the pool
THEN then an existing connection from the pool is returned and the request succeeds

Actual behavior

GIVEN there are database connection problems
AND the current number of connections is below MIN_SIZE
AND there are still valid connections in the pool
AND new connections cannot be created
WHEN a connection is requested from the pool
THEN no connection can be obtained and the request fails

How to Reproduce?

We reproduced the behaviour by changing the db password on a running application so the pool cannot create new connections and killing existing db sessions to bring the number of connections below MIN_SIZE.

However, the problem can be seen directly in the code: https://github.com/agroal/agroal/blob/4c4da6419652f772542e648581d0d8939c411b5e/agroal-pool/src/main/java/io/agroal/pool/ConnectionPool.java#L270-L323

  • if a new connection cannot be established, a TimeoutException is thrown and the code to take a connection from the pool is never reached
  • while it might be a good idea to trigger filling up the pool to MIN_SIZE, the current request should not incur the delay and probably the failure of obtaining a new connection but rather prefer using an existing connection - IMHO filling up should happen only concurrently

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@sschu sschu added the kind/bug Something isn't working label Feb 17, 2023
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 17, 2023

/cc @Sanne (agroal), @barreiro (agroal), @yrodiere (agroal)

@barreiro
Copy link
Contributor

thanks @sschu. The issue on the Agroal side is https://issues.redhat.com/projects/AG/issues/AG-201

@andreb89
Copy link

andreb89 commented Mar 8, 2023

As far as I can see, the fix for the connection problem was merged and the Jira issue resolved:

Fix Version/s of Agroal:
2.1, 1.17

@yrodiere
Copy link
Member

yrodiere commented Mar 8, 2023

We're using 2.0 at the moment in Quarkus. 2.1 was released just 15 hours ago, let's give @barreiro some time to update it in Quarkus ;)

@barreiro
Copy link
Contributor

barreiro commented Mar 8, 2023

I'm counting on dependabot to trigger the update. Usually doesn't take long.

@sschu
Copy link
Contributor Author

sschu commented Mar 10, 2023

Will this be backported to Quarkus 2.13 (for Keycloak)?

@quarkus-bot
Copy link

quarkus-bot bot commented Mar 10, 2023

@Sanne triage/backport* labels may not be added to an issue. Please add them to the corresponding pull request.

This message is automatically generated by a bot.

@Sanne
Copy link
Member

Sanne commented Mar 10, 2023

@sschu good idea - labelled it.

@sschu
Copy link
Contributor Author

sschu commented Mar 10, 2023

@Sanne Since the bot wouldn't create a PR, I did: #31761
I stayed with the 1.X branch of agroal using the backported fix for the issue in 1.17. I hope that's ok, otherwise we could also use the latest agroal version 2.1. I can also provide a PR for main and agroal version 2.1 should dependabot not make it. ;)

gsmet pushed a commit to gsmet/quarkus that referenced this issue Mar 10, 2023
@quarkus-bot quarkus-bot bot added this to the 3.0 - main milestone Mar 13, 2023
@gsmet gsmet modified the milestones: 3.0.0.Alpha6, 2.13.8.Final May 9, 2023
gsmet pushed a commit to gsmet/quarkus that referenced this issue May 9, 2023
Fixes quarkusio#31246

(cherry picked from commit 32cfabd)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/agroal kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants