Move README rendering off the web server

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))