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 DB connection leak #4

Merged
merged 7 commits into from
Jan 8, 2024
Merged

Fix DB connection leak #4

merged 7 commits into from
Jan 8, 2024

Conversation

martonarbocz
Copy link
Contributor

https://kaligo.slack.com/archives/C01LK7XLGE9/p1704682961528359
We discovered that using Sequel.connect to return the connection actually leaks it. Let's use a block instead.

  • sqlite depends on pkg-config, so I added that to bin/setup

Copy link

@Vuta Vuta left a comment

Choose a reason for hiding this comment

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

CI is failing, can you help fix it

bin/setup Show resolved Hide resolved
@martonarbocz
Copy link
Contributor Author

CI is failing, can you help fix it

Yeah I'm looking at it, quite weird because originally tests passed for me with the system Ruby, but they fail when 3.1.2.

Copy link
Collaborator

@hieuk09 hieuk09 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, could you add a test?

lib/sequel_data/migrate/migrator.rb Outdated Show resolved Hide resolved
@fill32
Copy link

fill32 commented Jan 8, 2024

how do we carry out the test? trigger the worker and see if there's any idle connection?

@martonarbocz
Copy link
Contributor Author

martonarbocz commented Jan 8, 2024

I wanted to just do expect ... not_change db.pool.size, but it seems that db connections stay constant during the test run, even when I deliberately created extra connections by not using a block 🤔

@martonarbocz
Copy link
Contributor Author

Maybe this is because of the simpler DB setup (just using SQLite)? Hieu, do you have any advice how to carry out the test reliably?

@Vuta
Copy link

Vuta commented Jan 8, 2024

@martonarbocz you can try assert the value of Sequel::DATABASES before and after migrator.migrate is called

@martonarbocz
Copy link
Contributor Author

@martonarbocz you can try assert the value of Sequel::DATABASES before and after migrator.migrate is called

Thank you sir, that works 💯

Comment on lines 122 to 126
before do
db.create_table(:data_migrations) do
String :version, null: false, primary: true
end
end
Copy link
Collaborator

@hieuk09 hieuk09 Jan 8, 2024

Choose a reason for hiding this comment

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

We don't need to create the table because the migrator will create the table if it does not exist.

@hieuk09 hieuk09 merged commit ac88aa9 into master Jan 8, 2024
8 checks passed
@hieuk09 hieuk09 deleted the fix/db_connection_leak branch January 8, 2024 14:08
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.

4 participants