Move README renderings off the web server

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. This
  commit does not include the migration, which must be deployed
  separately. This commit only includes code which works with or without
  the migration for deployment.
- `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: sgrif

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<Option<reqwest::Client>>,
 }
 
 impl Environment {
     pub fn new(
         index: Repository,
         credentials: Option<(String, String)>,
         connection_pool: DieselPool,
+        uploader: Uploader,
+        http_client: Option<reqwest::Client>,
     ) -> Self {
         Self {
             index: Mutex::new(index),
             credentials,
             connection_pool: AssertUnwindSafe(connection_pool),
+            uploader,
+            http_client: AssertUnwindSafe(http_client),
         }
     }
 
@@ -44,13 +52,27 @@ 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.
+    ///
+    /// # Panics
+    ///
+    /// Panics if no client was given when constructing the environment. This
+    /// will occur in tests unless `TestApp::with_proxy()` is called.
+    pub(crate) fn http_client(&self) -> &reqwest::Client {
+        self.http_client
+            .as_ref()
+            .expect("No HTTP client is configured. In tests, use `TestApp::with_proxy()`.")
+    }
 }

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,
+        Some(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,33 @@ 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 +194,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/middleware/run_pending_background_jobs.rs

@@ -22,7 +22,13 @@ impl Middleware for RunPendingBackgroundJobs {
 
         let connection_pool = app.diesel_database.clone();
         let repo = Repository::open(&app.config.index_location).expect("Could not clone index");
-        let environment = Environment::new(repo, None, app.diesel_database.clone());
+        let environment = Environment::new(
+            repo,
+            None,
+            app.diesel_database.clone(),
+            app.config.uploader.clone(),
+            app.http_client.clone(),
+        );
 
         let runner = Runner::builder(connection_pool, environment)
             // We only have 1 connection in tests, so trying to run more than

src/models/krate.rs

@@ -40,8 +40,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>,
@@ -58,8 +56,6 @@ type AllColumns = (
     crates::description,
     crates::homepage,
     crates::documentation,
-    crates::readme,
-    crates::readme_file,
     crates::license,
     crates::repository,
     crates::max_upload_size,
@@ -74,8 +70,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,
@@ -101,7 +95,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))