Auto merge of #1610 - sgrif:sg-test-connection, r=jtgeibel

Explicitly share a single connection in tests

This is required to move forward on #1588, I have spun it off into a standalone PR for ease of review, since it's isolated and a change that makes sense on its own. This requires #1609 and includes those commits

Right now the way our test suite works is that we set the database
connection pool size to 1, and tell that single connection to start a
test transaction. This works, but has the caveat that only a single
reference to the connection can be live at any given time.

This becomes a problem when we add background jobs into the mix, which
will always want to grab two connections from the pool (and we still
need them to be the same connection). The code that we end up running
looks something like:

```
// In request
let conn = pool.get();
insert_background_job(&conn)?;

// In middleware that runs jobs
for _ in 0..job_count {
    thread::spawn(|| {
        conn = pool.get();
        lock_job(&conn);
        // in actual job
        conn = pool.get();
        do_stuff(&conn);
        unlock_or_delete_job(&conn);
    });
}
```

Note that the worker and the job itself both need a connection. In order
to make this work, we need to change our pool to actually just be a
single connection given out multiple times in test mode. I've chosen to
use a reentrant mutex for this for two reasons:

1. We need to satisfy the `Sync` bound on `App`, even though we're only
  running in a single thread in tests.
2. The background worker actually does end up running in another thread,
  so we do actually need to be thread safe.

The result of this is that we can have the connection checked out more
than once at the same time, but we still can't be more relaxed about
when it gets dropped in our test code, since we need it to be unlocked
if we try to get a connection from another thread as the result of
running a background job. Our tests shouldn't know or care about whether
background jobs were run async or not, so they should assume every
request might run one.

The mutex guard holds a reference to the mutex, so this introduces a
lifetime constraint that hasn't existed since the Diesel port. The
return type of `req.db_conn()` is now effectively `&PgConnection`
instead of `Box<PgConnection>`. Since the method exists on `Request`,
this means it gets treated as an immutable borrow of the whole request.
The fixes are all pretty simple, and boil down to either cloning the
app and getting the DB directly from there, taking immutable references
instead of mutable ones, or dropping the connection when we're done with
it.

Author: bors

Cargo.lock

@@ -62,6 +62,7 @@ scheduled-thread-pool = "0.2.0"
 derive_deref = "1.0.0"
 reqwest = "0.9.1"
 tempdir = "0.3.7"
+parking_lot = "0.7.1"
 
 lettre = {git = "https://github.com/lettre/lettre", version = "0.9"}
 lettre_email = {git = "https://github.com/lettre/lettre", version = "0.9"}

Cargo.toml

@@ -73,6 +73,7 @@ impl App {
         let db_connection_timeout = match (env::var("DB_TIMEOUT"), config.env) {
             (Ok(num), _) => num.parse().expect("couldn't parse DB_TIMEOUT"),
             (_, Env::Production) => 10,
+            (_, Env::Test) => 1,
             _ => 30,
         };
 
@@ -88,7 +89,7 @@ impl App {
         let repo = git2::Repository::open(&config.git_repo_checkout).unwrap();
 
         App {
-            diesel_database: db::diesel_pool(&config.db_url, diesel_db_config),
+            diesel_database: db::diesel_pool(&config.db_url, config.env, diesel_db_config),
             github,
             session_key: config.session_key.clone(),
             git_repo: Mutex::new(repo),

src/app.rs

@@ -32,11 +32,11 @@ struct OwnerInvitation {
 
 /// Handles the `PUT /me/crate_owner_invitations/:crate_id` route.
 pub fn handle_invite(req: &mut dyn Request) -> CargoResult<Response> {
-    let conn = &*req.db_conn()?;
-
     let mut body = String::new();
     req.body().read_to_string(&mut body)?;
 
+    let conn = &*req.db_conn()?;
+
     let crate_invite: OwnerInvitation =
         serde_json::from_str(&body).map_err(|_| human("invalid json request"))?;
 
@@ -50,7 +50,7 @@ pub fn handle_invite(req: &mut dyn Request) -> CargoResult<Response> {
 }
 
 fn accept_invite(
-    req: &mut dyn Request,
+    req: &dyn Request,
     conn: &PgConnection,
     crate_invite: InvitationResponse,
 ) -> CargoResult<Response> {
@@ -88,7 +88,7 @@ fn accept_invite(
 }
 
 fn decline_invite(
-    req: &mut dyn Request,
+    req: &dyn Request,
     conn: &PgConnection,
     crate_invite: InvitationResponse,
 ) -> CargoResult<Response> {

src/controllers/crate_owner_invitation.rs

@@ -63,7 +63,7 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
     let categories = new_crate.categories.as_ref().map(|s| &s[..]).unwrap_or(&[]);
     let categories: Vec<_> = categories.iter().map(|k| &***k).collect();
 
-    let conn = req.db_conn()?;
+    let conn = app.diesel_database.get()?;
 
     let mut other_warnings = vec![];
     if !user.has_verified_email(&conn)? {

src/controllers/krate/publish.rs

@@ -171,7 +171,7 @@ pub fn search(req: &mut dyn Request) -> CargoResult<Response> {
 
     let badges = CrateBadge::belonging_to(&crates)
         .select((badges::crate_id, badges::all_columns))
-        .load::<CrateBadge>(&conn)?
+        .load::<CrateBadge>(&*conn)?
         .grouped_by(&crates)
         .into_iter()
         .map(|badges| badges.into_iter().map(|cb| cb.badge).collect());