Auto merge of #1745 - sgrif:sg-async-readme-renderings, r=jtgeibel

Move README rendering off the web server

:warning: This PR contains destructive migrations. The commits included must be deployed separately. :warning:

Rendering the README is a process that can take an arbitrarily long amount of time, and ends with uploading a file to S3 -- which also can take an arbitrarily long period of time. Since there are no README contents which we consider invalid (which I've indicated by changing the return type of the rendering functions to `String` instead of `Result`), there's no reason for us to do any of this work on the web server.

Just like the index updates, we can offload this to our worker to be completed at some point in the future. This does mean that a crate will render in our UI before the README has finished rendering. During this brief period, we will behave the same as if the crate had no README file, showing the description instead.

Eventually I think it'd make sense to have this job behave like the `render-readmes` worker, where it pulls the tar file down and reads the actual readme file. This would mean that Cargo can stop sending us the README as its own field when its already sending us a tar file which contains the README.  I've left it alone for now though, since there's no benefit while Cargo is already sending us the README, and we can't have it stop doing that until we move to publish v2.

A few implementation notes:

- The `readme` column on `crates` was never used for anything in Rust, but is used on the DB side to generate the search index. I've left the column in place, but removed most uses of it on the Rust side.
- The `readme_file` column on `crates` never used for anything.
- `background_jobs::Environment` gained two fields that are just straight up copied from `App`. I've opted to do this rather than just giving it `App`, since I do want to separate "the stuff you need to do background jobs" from "the stuff you need to be the web server". Since we do eventually plan on not having s3 uploads happen on the server, I want to bundle all of this into a single field and just have it live on `background_jobs::Environment` eventually.
- I did some general cleanup of `krate::publish` while I was in there, removed a lot of pre-1.0 style code.
- I've left the `render-readmes` binary alone for now since it's untested. I'll change it to just queue up jobs once we get to the point where jobs are doing all the tar file work as well.

Author: bors

migrations/2019-05-13-192055_remove_readme_from_crates/down.sql

@@ -0,0 +1,2 @@
+-- This file should undo anything in `up.sql`
+ALTER TABLE crates ADD COLUMN readme_file VARCHAR;

migrations/2019-05-13-192055_remove_readme_from_crates/up.sql

@@ -0,0 +1,2 @@
+-- Your SQL goes here
+ALTER TABLE crates DROP COLUMN readme_file;

src/app.rs

@@ -33,7 +33,7 @@ pub struct App {
     /// In production this shares a single connection pool across requests.  In tests
     /// this is either None (in which case any attempt to create an outgoing connection
     /// will panic) or a `Client` configured with a per-test replay proxy.
-    http_client: Option<Client>,
+    pub(crate) http_client: Option<Client>,
 }
 
 impl App {

src/background_jobs.rs

@@ -1,8 +1,10 @@
 use std::panic::AssertUnwindSafe;
 use std::sync::{Mutex, MutexGuard, PoisonError};
+use swirl::PerformError;
 
 use crate::db::{DieselPool, DieselPooledConn};
 use crate::git::Repository;
+use crate::uploaders::Uploader;
 use crate::util::errors::{CargoErrToStdErr, CargoResult};
 
 impl<'a> swirl::db::BorrowedConnection<'a> for DieselPool {
@@ -23,18 +25,24 @@ pub struct Environment {
     pub credentials: Option<(String, String)>,
     // FIXME: https://github.com/sfackler/r2d2/pull/70
     pub connection_pool: AssertUnwindSafe<DieselPool>,
+    pub uploader: Uploader,
+    http_client: AssertUnwindSafe<reqwest::Client>,
 }
 
 impl Environment {
     pub fn new(
         index: Repository,
         credentials: Option<(String, String)>,
         connection_pool: DieselPool,
+        uploader: Uploader,
+        http_client: reqwest::Client,
     ) -> Self {
         Self {
             index: Mutex::new(index),
             credentials,
             connection_pool: AssertUnwindSafe(connection_pool),
+            uploader,
+            http_client: AssertUnwindSafe(http_client),
         }
     }
 
@@ -44,13 +52,20 @@ impl Environment {
             .map(|(u, p)| (u.as_str(), p.as_str()))
     }
 
-    pub fn connection(&self) -> CargoResult<DieselPooledConn<'_>> {
-        self.connection_pool.0.get().map_err(Into::into)
+    pub fn connection(&self) -> Result<DieselPooledConn<'_>, PerformError> {
+        self.connection_pool
+            .get()
+            .map_err(|e| CargoErrToStdErr(e).into())
     }
 
     pub fn lock_index(&self) -> CargoResult<MutexGuard<'_, Repository>> {
         let repo = self.index.lock().unwrap_or_else(PoisonError::into_inner);
         repo.reset_head()?;
         Ok(repo)
     }
+
+    /// Returns a client for making HTTP requests to upload crate files.
+    pub(crate) fn http_client(&self) -> &reqwest::Client {
+        &self.http_client
+    }
 }

src/bin/background-worker.rs

@@ -35,7 +35,13 @@ fn main() {
 
     let repository = Repository::open(&config.index_location).expect("Failed to clone index");
 
-    let environment = Environment::new(repository, credentials, db_pool.clone());
+    let environment = Environment::new(
+        repository,
+        credentials,
+        db_pool.clone(),
+        config.uploader,
+        reqwest::Client::new(),
+    );
 
     let runner = swirl::Runner::builder(db_pool, environment)
         .thread_count(1)

src/bin/render-readmes.rs

@@ -114,7 +114,7 @@ fn main() {
         let mut tasks = Vec::with_capacity(page_size as usize);
         for (version, krate_name) in versions {
             let config = config.clone();
-            version.record_readme_rendering(&conn).unwrap_or_else(|_| {
+            Version::record_readme_rendering(version.id, &conn).unwrap_or_else(|_| {
                 panic!(
                     "[{}-{}] Couldn't record rendering time",
                     krate_name, version.num
@@ -215,7 +215,6 @@ fn get_readme(
                 .map_or("README.md", |e| &**e),
             manifest.package.repository.as_ref().map(|e| &**e),
         )
-        .unwrap_or_else(|_| panic!("[{}-{}] Couldn't render README", krate_name, version.num))
     };
     return Some(rendered);
 

src/controllers/krate/publish.rs

@@ -1,7 +1,6 @@
 //! Functionality related to publishing a new crate or version of a crate.
 
 use hex::ToHex;
-use std::collections::HashMap;
 use std::sync::Arc;
 use swirl::Job;
 
@@ -10,8 +9,8 @@ use crate::git;
 use crate::models::dependency;
 use crate::models::{Badge, Category, Keyword, NewCrate, NewVersion, Rights, User};
 use crate::render;
-use crate::util::{internal, CargoError, ChainError, Maximums};
 use crate::util::{read_fill, read_le_u32};
+use crate::util::{CargoError, ChainError, Maximums};
 use crate::views::{EncodableCrateUpload, GoodCrate, PublishWarnings};
 
 /// Handles the `PUT /crates/new` route.
@@ -39,29 +38,6 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
 
     let (new_crate, user) = parse_new_headers(req)?;
 
-    let name = &*new_crate.name;
-    let vers = &*new_crate.vers;
-    let links = new_crate.links.clone();
-    let repo = new_crate.repository.as_ref().map(|s| &**s);
-    let features = new_crate
-        .features
-        .iter()
-        .map(|(k, v)| {
-            (
-                k[..].to_string(),
-                v.iter().map(|v| v[..].to_string()).collect(),
-            )
-        })
-        .collect::<HashMap<String, Vec<String>>>();
-    let keywords = new_crate
-        .keywords
-        .as_ref()
-        .map(|kws| kws.iter().map(|kw| &***kw).collect())
-        .unwrap_or_else(Vec::new);
-
-    let categories = new_crate.categories.as_ref().map(|s| &s[..]).unwrap_or(&[]);
-    let categories: Vec<_> = categories.iter().map(|k| &***k).collect();
-
     let conn = app.diesel_database.get()?;
 
     let verified_email_address = user.verified_email(&conn)?;
@@ -75,15 +51,34 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
     // Create a transaction on the database, if there are no errors,
     // commit the transactions to record a new or updated crate.
     conn.transaction(|| {
+        let name = new_crate.name;
+        let vers = &*new_crate.vers;
+        let links = new_crate.links;
+        let repo = new_crate.repository;
+        let features = new_crate
+            .features
+            .into_iter()
+            .map(|(k, v)| (k.0, v.into_iter().map(|v| v.0).collect()))
+            .collect();
+        let keywords = new_crate
+            .keywords
+            .iter()
+            .map(|s| s.as_str())
+            .collect::<Vec<_>>();
+        let categories = new_crate
+            .categories
+            .iter()
+            .map(|s| s.as_str())
+            .collect::<Vec<_>>();
+
         // Persist the new crate, if it doesn't already exist
         let persist = NewCrate {
-            name,
+            name: &name,
             description: new_crate.description.as_ref().map(|s| &**s),
             homepage: new_crate.homepage.as_ref().map(|s| &**s),
             documentation: new_crate.documentation.as_ref().map(|s| &**s),
             readme: new_crate.readme.as_ref().map(|s| &**s),
-            readme_file: new_crate.readme_file.as_ref().map(|s| &**s),
-            repository: repo,
+            repository: repo.as_ref().map(String::as_str),
             license: new_crate.license.as_ref().map(|s| &**s),
             max_upload_size: None,
         };
@@ -101,7 +96,7 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
             ));
         }
 
-        if &krate.name != name {
+        if krate.name != *name {
             return Err(human(&format_args!(
                 "crate was previously named `{}`",
                 krate.name
@@ -163,31 +158,30 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
         let ignored_invalid_badges = Badge::update_crate(&conn, &krate, new_crate.badges.as_ref())?;
         let max_version = krate.max_version(&conn)?;
 
-        // Render the README for this crate
-        let readme = match new_crate.readme.as_ref() {
-            Some(readme) => Some(render::readme_to_html(
-                &**readme,
-                new_crate.readme_file.as_ref().map_or("README.md", |s| &**s),
+        if let Some(readme) = new_crate.readme {
+            render::render_and_upload_readme(
+                version.id,
+                readme,
+                new_crate
+                    .readme_file
+                    .unwrap_or_else(|| String::from("README.md")),
                 repo,
-            )?),
-            None => None,
-        };
+            )
+            .enqueue(&conn)
+            .map_err(|e| CargoError::from_std_error(e))?;
+        }
 
-        // Upload the crate, return way to delete the crate from the server
-        // If the git commands fail below, we shouldn't keep the crate on the
-        // server.
-        let (cksum, mut crate_bomb, mut readme_bomb) = app
+        let cksum = app
             .config
             .uploader
-            .upload_crate(req, &krate, readme, maximums, vers)?;
-        version.record_readme_rendering(&conn)?;
+            .upload_crate(req, &krate, maximums, vers)?;
 
         let mut hex_cksum = String::new();
         cksum.write_hex(&mut hex_cksum)?;
 
         // Register this crate in our local git repo.
         let git_crate = git::Crate {
-            name: name.to_string(),
+            name: name.0,
             vers: vers.to_string(),
             cksum: hex_cksum,
             features,
@@ -197,17 +191,7 @@ pub fn publish(req: &mut dyn Request) -> CargoResult<Response> {
         };
         git::add_crate(git_crate)
             .enqueue(&conn)
-            .map_err(|e| CargoError::from_std_error(e))
-            .chain_error(|| {
-                internal(&format_args!(
-                    "could not add crate `{}` to the git repo",
-                    name
-                ))
-            })?;
-
-        // Now that we've come this far, we're committed!
-        crate_bomb.path = None;
-        readme_bomb.path = None;
+            .map_err(|e| CargoError::from_std_error(e))?;
 
         // The `other` field on `PublishWarnings` was introduced to handle a temporary warning
         // that is no longer needed. As such, crates.io currently does not return any `other`

src/git.rs

@@ -165,7 +165,7 @@ pub fn yank(
     let repo = env.lock_index().map_err(std_error_no_send)?;
     let dst = repo.index_file(&krate);
 
-    let conn = env.connection().map_err(std_error_no_send)?;
+    let conn = env.connection()?;
 
     conn.transaction(|| {
         let yanked_in_db = versions::table

src/models/krate.rs

@@ -41,8 +41,6 @@ pub struct Crate {
     pub description: Option<String>,
     pub homepage: Option<String>,
     pub documentation: Option<String>,
-    pub readme: Option<String>,
-    pub readme_file: Option<String>,
     pub license: Option<String>,
     pub repository: Option<String>,
     pub max_upload_size: Option<i32>,
@@ -59,8 +57,6 @@ type AllColumns = (
     crates::description,
     crates::homepage,
     crates::documentation,
-    crates::readme,
-    crates::readme_file,
     crates::license,
     crates::repository,
     crates::max_upload_size,
@@ -75,8 +71,6 @@ pub const ALL_COLUMNS: AllColumns = (
     crates::description,
     crates::homepage,
     crates::documentation,
-    crates::readme,
-    crates::readme_file,
     crates::license,
     crates::repository,
     crates::max_upload_size,
@@ -100,7 +94,6 @@ pub struct NewCrate<'a> {
     pub homepage: Option<&'a str>,
     pub documentation: Option<&'a str>,
     pub readme: Option<&'a str>,
-    pub readme_file: Option<&'a str>,
     pub repository: Option<&'a str>,
     pub max_upload_size: Option<i32>,
     pub license: Option<&'a str>,

src/models/version.rs

@@ -99,12 +99,12 @@ impl Version {
             })
     }
 
-    pub fn record_readme_rendering(&self, conn: &PgConnection) -> QueryResult<usize> {
+    pub fn record_readme_rendering(version_id_: i32, conn: &PgConnection) -> QueryResult<usize> {
         use crate::schema::readme_renderings::dsl::*;
         use diesel::dsl::now;
 
         diesel::insert_into(readme_renderings)
-            .values(version_id.eq(self.id))
+            .values(version_id.eq(version_id_))
             .on_conflict(version_id)
             .do_update()
             .set(rendered_at.eq(now))