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

Fix connections leaking on high load. #290

Merged
merged 10 commits into from May 31, 2022
Merged

Fix connections leaking on high load. #290

merged 10 commits into from May 31, 2022

Conversation

sbmpost
Copy link
Contributor

@sbmpost sbmpost commented Apr 28, 2022 •
edited

  1. Moves the responsibility of connection create timeout completely to the connection.
  2. By doing so, allows us to close the underlying Netty Channel if the creation times out.

@sbmpost sbmpost mentioned this pull request Apr 28, 2022
Closed
@sbmpost sbmpost force-pushed the 288-fix-connections-leaking branch from 798288a to 814c385 Compare May 2, 2022 16:11
@sbmpost sbmpost changed the title Fix PostgreSQL connections leaking. TODO: MySQL Fix connections leaking on high load. May 2, 2022
@sbmpost
Copy link
Contributor Author

sbmpost commented May 2, 2022

@oshai
I have completed my PR. It would be great if you can have a look.

oshai
oshai reviewed May 3, 2022
View reviewed changes
Copy link
Contributor

@oshai oshai left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

db-async-common/src/main/java/com/github/jasync/sql/db/Configuration.kt Outdated
@@ -50,6 +51,7 @@ data class Configuration @JvmOverloads constructor(
val maximumMessageSize: Int = 16777216,
val allocator: ByteBufAllocator = PooledByteBufAllocator.DEFAULT,
val connectionTimeout: Int = 5000,
val createTimeout: Duration = Duration.ofMillis(5000),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is exactly as connectionTimeout

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed, will fix this

mysql-async/src/test/java/com/github/jasync/sql/db/MySQLSlowConnectionDelegate.kt Outdated
delegate.onError(message)

override fun onOk(message: OkMessage) {
Thread.sleep(onOkSlowdownInMillis)
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to make the network connection, but to simulate a significant delay in the response of MySQL server so that the create/connectionTimeout kicks in. As far as I can tell, the Ok message is similar to the PostgreSQL ReadyForQuery message (see issue #288)

mysql-async/src/test/java/com/github/jasync/sql/db/MySQLSlowConnectionDelegate.kt Outdated
class MySQLSlowConnectionDelegate(
private val delegate: MySQLHandlerDelegate,
private val onOkSlowdownInMillis: Long
) : MySQLHandlerDelegate {
Copy link
Contributor

Choose a reason for hiding this comment

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

kotlin has a delegate feature, maybe you can use that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That certainly removed a lot of boilerplate code, thanks!

pool-async/src/main/java/com/github/jasync/sql/db/pool/ActorBasedObjectPool.kt Outdated
@@ -337,17 +336,6 @@ private class ObjectPoolActor<T : PooledObject>(
}
}

private fun checkItemsInCreationForTimeout() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would keep that as a safety mechanism, maybe with a bigger threshold like createTimeout * 2 (or + 20 seconds)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like a good idea to keep things backwards compatible. I have restored it.

@sbmpost
Copy link
Contributor Author

sbmpost commented May 4, 2022

@oshai
I have gone over all review comments and made the necessary changes.

@sbmpost
Copy link
Contributor Author

sbmpost commented May 9, 2022

@oshai can we consider to merge this?

@oshai
Copy link
Contributor

oshai commented May 9, 2022

I am not available in the next two weeks so will complete the review once back.

@oshai
Copy link
Contributor

oshai commented May 20, 2022

The CI fails on formatting (also failed on using old kotlin version without last ,). I will fix it later today or you can do that - I want to make sure CI pass before merging.

@oshai
Copy link
Contributor

oshai commented May 20, 2022

The change cause some tests fail. Maybe the timeout is misconfigured or there is another issue. I would appreciate if you can take a look.

@sbmpost
Copy link
Contributor Author

sbmpost commented May 31, 2022 •
edited

@oshai
Previously the connectionTimeout setting was passed to the NIO java socket library which due to its non-blocking nature never really times out. But now that we set up our own timer as well in order to properly cleanup the channel before the pool forceably destroys the connection (with createTimeout=2*connectionTimeout), the connectionTimeout actually works as it is supposed to. This however made a test fail that demanded a 1 second connectionTimeout. By removing this 1 second check, the test now succeeds.

@sbmpost
Copy link
Contributor Author

sbmpost commented May 31, 2022

@oshai I had to fix the same thing for postgresql. I can't restart the test myself, but I think all should be well now.

@oshai
Copy link
Contributor

oshai commented May 31, 2022

ok thanks! I will merge once all tests pass.

@oshai oshai merged commit 936e26d into jasync-sql:master May 31, 2022
@oshai
Copy link
Contributor

oshai commented May 31, 2022

Thanks for the PR! I will create a new version soon.

@oshai
Copy link
Contributor

oshai commented May 31, 2022

releasing it now on v2.0.8

oshai pushed a commit that referenced this pull request Sep 18, 2022
@sbmpost @oshai
Fix connections leaking on high load. (#290)
4859991 
Moves the responsibility of connection create timeout completely to the connection.
By doing so, allows us to close the underlying Netty Channel if the creation times out.

Also:
* Lower connectionTimeout in the unit tests
* Fix unit tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oshai oshai oshai left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sbmpost @oshai