Skip to content

Use a single connection pool across the whole application #843

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

Merged
merged 9 commits into from
Jun 27, 2020

Conversation

pietroalbini
Copy link
Member

This PR changes how database connections are handled in the docs.rs application:

  • Some code has been shuffled around, moving web::InjectExtensions to its own file and web::pool::Pool to db::pool::Pool.
  • The configuration for the database is now stored in Config instead of being loaded from the environment each time a connection is created.
  • The application is changed to create a single instance of Pool, which is created in the cratesfyi CLI and passed around. One result of this is that the frontend and the builds will use the same pool, instead of having the builds create standalone connections when needed.
  • db::connect_db() is removed, forcing every connection to go through the database pool.

This PR is part of my refactorings to make buildtests feasible, and is best reviewed commit by commit. Unfortunately the last commit is quite big, but there was no way to split it.

@Nemo157
Copy link
Member

Nemo157 commented Jun 21, 2020

How certain are we that the pool is fork-safe? (Given that we initialise it and pass it off to start_daemon which may then fork the process).

@pietroalbini
Copy link
Member Author

Good point. I'd be tempted to just remove the forking code: we run docs.rs on production with systemd, and tbh everyone should just do that.

@Nemo157
Copy link
Member

Nemo157 commented Jun 21, 2020

+1, I think self-backgrounding is an antipattern that can be dropped. Last I asked about it @jyn514 said it was used in production still, so might need a reconfiguration there to stop using it.

@pietroalbini
Copy link
Member Author

Opened #844 to remove forking.

@jyn514
Copy link
Member

jyn514 commented Jun 21, 2020

How certain are we that the pool is fork-safe? (Given that we initialise it and pass it off to start_daemon which may then fork the process).

Technically all IO is unsafe unless it's been explicitly whitelisted:

   *  After  a  fork()  in  a multithreaded program, the child can safely call
     only async-signal-safe functions (see signal-safety(7)) until such  time
     as it calls execve(2).

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Mostly minor nits, overall this looks great :)

@Nemo157
Copy link
Member

Nemo157 commented Jun 22, 2020

Technically all IO is unsafe unless it's been explicitly whitelisted:

I was more thinking that if the pool spawns a background thread to deal with closing old connections that thread won't make it across the fork.

@pietroalbini
Copy link
Member Author

Replied to or addressed all review comments.

@jyn514 jyn514 added S-waiting-on-author Status: This PR is incomplete or needs to address review comments P-high High priority labels Jun 26, 2020
@jyn514
Copy link
Member

jyn514 commented Jun 26, 2020

Marking as P-high since it blocks #822.

@pietroalbini
Copy link
Member Author

Rebased on top of the latest master and fixed the last concern in the last commit.

r? @jyn514

@pietroalbini pietroalbini added S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed and removed S-waiting-on-author Status: This PR is incomplete or needs to address review comments labels Jun 27, 2020
@jyn514 jyn514 merged commit ac33e2d into rust-lang:master Jun 27, 2020
@pietroalbini pietroalbini deleted the one-pool branch June 27, 2020 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-high High priority S-waiting-on-review Status: This pull request has been implemented and needs to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants