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

fix: EXPOSED-133 Suspend transactions blocking Hikari connection pool #1837

Merged
merged 3 commits into from
Aug 22, 2023

Conversation

bog-walk
Copy link
Member

@bog-walk bog-walk commented Aug 20, 2023

If the maximumPoolSize of a HikariDataSource is exceeded, any active connections made through newSuspendedTransaction() are not released back into the pool after job completion. This leads to a connection timeout exception when attempting to get more connections.

resetIfClosed(), invoked in suspendedTransactionAsyncInternal(), calls getConnection() to check if a transaction is closed (may happen after a repetition attempt fails), so that it can be properly reset for another attempt. This blocks the connections being released.

The function was introduced in PR #1774 and should not be necessary for the first loop, as the transaction is created with an open connection.

Now resetIfClosed() is only called on subsequent repetition attempt loops.

Unit test:

  • No unit test has been included as it would require including a permanent dependency on Hikari CP in the exposed-tests module.
  • To test on the branch, include the following in exposed-tests/build.gradle.kts, before running the test provided in the original YT issue:
implementation("com.zaxxer:HikariCP:5.0.1")
implementation("com.h2database", "h2", Versions.h2)
// compileOnly("com.h2database", "h2", Versions.h2)

If the maximumPoolSize of a HikariDataSource is reached, the active connections
made through newSuspendedTransaction() are not being released back into the pool,
leading to a connection timeout exception when attempting to get more connections.

resetIfClosed(), in suspendedTransactionAsyncInternal(), calls getConnection() to
check if the transaction is closed (may happen after a repetition attempt), so
that it can be properly reset for a new attempt.

This blocks the connections being released and should not be necessary for the
first loop, as the created transaction has an open connection.

resetIfClosed() is now only accessed on subsequent repetition attempt loops.
@bog-walk bog-walk requested review from e5l and joc-a August 20, 2023 20:16
Copy link
Member

@e5l e5l left a comment

Choose a reason for hiding this comment

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

I would add h2 dependency in exposed-tests to make a test, it's not a published module so it should be fine

Add Hikari CP dependency in exposed-tests module.
Add new test suite for Hikari CP.
Switch H2 test dependency to compileOnly to avoid exposed-java-time build failures.
@bog-walk
Copy link
Member Author

@e5l Switching h2 dependency from compileOnly to implementation causes a build fail on multiple tests in exposed-java-time, so that dependency hasn't been changed.

A dependency on Hikari CP was still included with a new unit test by letting HikariDataSource resolve the H2 driver itself instead of configuring driverClassName.

@bog-walk bog-walk requested a review from e5l August 21, 2023 19:33
@bog-walk bog-walk merged commit b00f1d8 into main Aug 22, 2023
3 checks passed
@bog-walk bog-walk deleted the bog-walk/fix-suspend-connection-reset branch August 22, 2023 13:29
saral pushed a commit to saral/Exposed that referenced this pull request Oct 3, 2023
…JetBrains#1837)

* fix: EXPOSED-133 Suspend transactions blocking Hikari connection pool

If the maximumPoolSize of a HikariDataSource is reached, the active connections
made through newSuspendedTransaction() are not being released back into the pool,
leading to a connection timeout exception when attempting to get more connections.

resetIfClosed(), in suspendedTransactionAsyncInternal(), calls getConnection() to
check if the transaction is closed (may happen after a repetition attempt), so
that it can be properly reset for a new attempt.

This blocks the connections being released and should not be necessary for the
first loop, as the created transaction has an open connection.

resetIfClosed() is now only accessed on subsequent repetition attempt loops.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants