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

Use a singleton for the DB connection object #5257

Merged
merged 1 commit into from
Oct 31, 2024
Merged

Conversation

Vinnl
Copy link
Collaborator

@Vinnl Vinnl commented Oct 28, 2024

References:

Jira: MNTOR-3606
Figma:

Description

This is the riskier sibling to #5254, where I'm trying to figure out what could be causing the monthly-activity-free cronjob to be hanging.

In this PR, I call destroy() on the connection object when it's done - but more importantly, I have every db/tables module re-use the same connection object, to avoid having to do this:

await knexSubscribers.destroy();
await knexEmailAddresses.destroy();
await knexHibp.destroy();

I think this should be safe, and in practice, the connections should be pooled already, but there might always be unforeseen effects, so please take some time to think about potential problems and/or ways to catch any that might occur.

@Vinnl Vinnl added the Review: XS Code review time: up to 30min label Oct 28, 2024
@Vinnl Vinnl requested review from rhelmer and mansaj October 28, 2024 16:15
@Vinnl Vinnl self-assigned this Oct 28, 2024
Copy link

Copy link
Collaborator

@rhelmer rhelmer left a comment

Choose a reason for hiding this comment

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

Yes I think this is a better approach here. Could you please file a ticket to refactor the rest of the code to use this approach?

src/db/connect.ts Show resolved Hide resolved
@Vinnl
Copy link
Collaborator Author

Vinnl commented Oct 31, 2024

@rhelmer

Could you please file a ticket to refactor the rest of the code to use this approach?

The risky bit is that this already ensures all /db/tables/* files to re-use that singleton (because they call the createDbConnection function that I modified), so I think the rest of the code already is adapted. Or do you mean updating the rest of the .destroy() calls?

Copy link

@Vinnl Vinnl merged commit 8d5fdad into main Oct 31, 2024
16 checks passed
@Vinnl Vinnl deleted the MNTOR-3606-db-singleton branch October 31, 2024 11:10
Copy link

Cleanup completed - database 'blurts-server-pr-5257' destroyed, cloud run service 'blurts-server-pr-5257' destroyed

@rhelmer
Copy link
Collaborator

rhelmer commented Oct 31, 2024

@rhelmer

Could you please file a ticket to refactor the rest of the code to use this approach?

The risky bit is that this already ensures all /db/tables/* files to re-use that singleton (because they call the createDbConnection function that I modified), so I think the rest of the code already is adapted. Or do you mean updating the rest of the .destroy() calls?

Yes thinking of e.g.

await knexSubscribers.destroy();
await knexEmailAddresses.destroy();
await knexHibp.destroy();
- I wonder if we could expose this destroy method to the scripts so they have a simpler way to make sure everything is closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Review: XS Code review time: up to 30min
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants