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

Allow closing idle connexion after a period #523

Closed
ArthurKnoep opened this issue Apr 2, 2021 · 7 comments
Closed

Allow closing idle connexion after a period #523

ArthurKnoep opened this issue Apr 2, 2021 · 7 comments
Assignees
Labels
feat New feature or request. help wanted We are looking for help on this one.

Comments

@ArthurKnoep
Copy link
Contributor

ArthurKnoep commented Apr 2, 2021

Describe the bug

Currently, the SQL DSN flag max_conn_lifetime allow to modify the value passed to sql.SetConnMaxLifetime (https://golang.org/pkg/database/sql/#DB.SetConnMaxLifetime), which only close in-use connexion after the period indicated. I think we should also be able to set sql.SetConnMaxIdleTime (https://golang.org/pkg/database/sql/#DB.SetConnMaxIdleTime), to be able to close idle connexions after a period.

(The code in question is located in the repo ory/x: https://github.com/ory/x/blob/400477d52ed124d603ce96849ae3bdb549e31b47/sqlcon/connector.go#L186)

I think that we should add a new flag (something like max_idle_conn_lifetime) instead of re-using the current max_conn_lifetime flag in order to have a more granular configuration to the database connexion pool.

My main motivation for having such a flag is to have Keto close all its connexions to the database after a period of inactivity so that our RDS cluster can scale down.

Reproducing the bug

Steps to reproduce the behavior:

  1. Run Keto with postgres
  2. In the DSN specify a max_conn_lifetime to 30s for example

Expected behavior

I expect Keto to close all connexion to the database 30 seconds after the last call to the database

Environment

  • Version: v0.5.2
  • Postgres version: AWS Aurora Serveless RDS 10.12

Additional context

I've only tested this "bug" with Postgres SQL but I assume there will be the same behaviour with MySQL as it use the same underlaying library.

Also this issue follow a discussion on the Ory Slack: https://ory-community.slack.com/archives/C012RBZFMDG/p1617194580015600

@ArthurKnoep
Copy link
Contributor Author

Also if this feature get approved, I'm availaible to implement it

@aeneasr
Copy link
Member

aeneasr commented Apr 2, 2021

SGTM, contribs welcomed!

@aeneasr aeneasr added feat New feature or request. help wanted We are looking for help on this one. labels Apr 2, 2021
@ArthurKnoep ArthurKnoep changed the title Allow closing idle connexion after a periode Allow closing idle connexion after a period Apr 2, 2021
@ArthurKnoep
Copy link
Contributor Author

Ok so for the follow up, I've opened an issue on gobufallo/pop (gobuffalo/pop#632)

Also would it be possible to be assigned on this issue ?

@aeneasr
Copy link
Member

aeneasr commented Apr 4, 2021

Of course!

I am also a pop maintainer so could review and merge your PR there :)

@ArthurKnoep
Copy link
Contributor Author

I've opened two PRs, one for x as I need to update the internal function that parse the sql flags, and a second one for keto that includes the required change to make the new flag work.

I also plan to open a PR for hydra in the future

@ArthurKnoep
Copy link
Contributor Author

ArthurKnoep commented Jun 1, 2021

Roses are red, violets are blue, all the PRs are opened

@aeneasr aeneasr closed this as completed in 50a8623 Jun 2, 2021
@ArthurKnoep
Copy link
Contributor Author

There is still a PR opened for hydra however as this issue is opened in the keto repository it's ok close here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request. help wanted We are looking for help on this one.
Projects
None yet
Development

No branches or pull requests

2 participants