Initialize a single reqwest::Client for the server process

A `reqwest::Client` represents a pool of connections and should be
reused.  For the `server` and `render-readme` binaries, a single pool
is now used for all requests.

This should reduce allocation activity in production for routes that
make an outgoing HTTP connection.  Each initialization costs 30,255
allocations and 2.0 MB of allocated data.  This work is also done on a
new thread which will map it to a random jemalloc arena.  The
initialization is now done once when the server is booted and any
remaining per-request allocations will occur on a smaller thread pool.

This also cleans up how proxying is implemented when running tests.
Previously, the configuration was stored in the S3 `Uploader` but this
resulted in test failures when calling functions (i.e. `crate_location`
and `readme_location`) that do not make an HTTP request.

Author: jtgeibel

src/app.rs

@@ -1,9 +1,10 @@
 //! Application-wide components in a struct accessible from each request
 
-use crate::{db, util::CargoResult, Config, Env};
+use crate::{db, Config, Env};
 use std::{path::PathBuf, sync::Arc, time::Duration};
 
 use diesel::r2d2;
+use reqwest::Client;
 use scheduled_thread_pool::ScheduledThreadPool;
 
 /// The `App` struct holds the main components of the application like
@@ -26,17 +27,24 @@ pub struct App {
 
     /// The server configuration
     pub config: Config,
+
+    /// A configured client for outgoing HTTP requests
+    ///
+    /// 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>,
 }
 
 impl App {
-    /// Creates a new `App` with a given `Config`
+    /// Creates a new `App` with a given `Config` and an optional HTTP `Client`
     ///
     /// Configures and sets up:
     ///
     /// - GitHub OAuth
     /// - Database connection pools
-    /// - A `git2::Repository` instance from the index repo checkout (that server.rs ensures exists)
-    pub fn new(config: &Config) -> App {
+    /// - Holds an HTTP `Client` and associated connection pool, if provided
+    pub fn new(config: &Config, http_client: Option<Client>) -> App {
         let mut github = oauth2::Config::new(
             &config.gh_client_id,
             &config.gh_client_secret,
@@ -90,19 +98,22 @@ impl App {
             session_key: config.session_key.clone(),
             git_repo_checkout: config.git_repo_checkout.clone(),
             config: config.clone(),
+            http_client,
         }
     }
 
     /// Returns a client for making HTTP requests to upload crate files.
     ///
-    /// The handle will go through a proxy if the uploader being used has specified one, which
-    /// is only done in tests with `TestApp::with_proxy()` in order to be able to record and
-    /// inspect the HTTP requests that tests make.
-    pub fn http_client(&self) -> CargoResult<reqwest::Client> {
-        let mut builder = reqwest::Client::builder();
-        if let Some(proxy) = self.config.uploader.proxy() {
-            builder = builder.proxy(reqwest::Proxy::all(proxy)?);
-        }
-        Ok(builder.build()?)
+    /// The client will go through a proxy if the application was configured via
+    /// `TestApp::with_proxy()`.
+    ///
+    /// # Panics
+    ///
+    /// Panics if the application was not initialized with a client.  This should only occur in
+    /// tests that were not properly initialized.
+    pub fn http_client(&self) -> &Client {
+        self.http_client
+            .as_ref()
+            .expect("No HTTP client is configured.  In tests, use `TestApp::with_proxy()`.")
     }
 }

src/bin/render-readmes.rs

@@ -21,6 +21,7 @@ use chrono::{TimeZone, Utc};
 use diesel::{dsl::any, prelude::*};
 use docopt::Docopt;
 use flate2::read::GzDecoder;
+use reqwest::Client;
 use tar::{self, Archive};
 
 const DEFAULT_PAGE_SIZE: usize = 25;
@@ -94,6 +95,8 @@ fn main() {
         total_pages + 1
     };
 
+    let client = Client::new();
+
     for (page_num, version_ids_chunk) in version_ids.chunks(page_size).enumerate() {
         println!(
             "= Page {} of {} ==================================",
@@ -117,22 +120,18 @@ fn main() {
                     krate_name, version.num
                 )
             });
+            let client = client.clone();
             let handle = thread::spawn(move || {
                 println!("[{}-{}] Rendering README...", krate_name, version.num);
-                let readme = get_readme(&config, &version, &krate_name);
+                let readme = get_readme(&config, &client, &version, &krate_name);
                 if readme.is_none() {
                     return;
                 }
                 let readme = readme.unwrap();
                 let readme_path = format!("readmes/{0}/{0}-{1}.html", krate_name, version.num);
                 config
                     .uploader
-                    .upload(
-                        &reqwest::Client::new(),
-                        &readme_path,
-                        readme.into_bytes(),
-                        "text/html",
-                    )
+                    .upload(&client, &readme_path, readme.into_bytes(), "text/html")
                     .unwrap_or_else(|_| {
                         panic!(
                             "[{}-{}] Couldn't upload file to S3",
@@ -151,12 +150,17 @@ fn main() {
 }
 
 /// Renders the readme of an uploaded crate version.
-fn get_readme(config: &Config, version: &Version, krate_name: &str) -> Option<String> {
+fn get_readme(
+    config: &Config,
+    client: &Client,
+    version: &Version,
+    krate_name: &str,
+) -> Option<String> {
     let location = config
         .uploader
-        .crate_location(krate_name, &version.num.to_string())?;
+        .crate_location(krate_name, &version.num.to_string());
 
-    let mut response = match reqwest::get(&location) {
+    let mut response = match client.get(&location).send() {
         Ok(r) => r,
         Err(err) => {
             println!(

src/bin/server.rs

@@ -9,6 +9,7 @@ use std::{
 
 use civet::Server as CivetServer;
 use conduit_hyper::Service as HyperService;
+use reqwest::Client;
 
 enum Server {
     Civet(CivetServer),
@@ -24,7 +25,9 @@ fn main() {
     env_logger::init();
 
     let config = cargo_registry::Config::default();
-    let app = App::new(&config);
+    let client = Client::new();
+
+    let app = App::new(&config, Some(client));
     let app = cargo_registry::build_handler(Arc::new(app));
 
     // On every server restart, ensure the categories available in the database match

src/config.rs

@@ -68,7 +68,6 @@ impl Default for Config {
                         &api_protocol,
                     ),
                     cdn: dotenv::var("S3_CDN").ok(),
-                    proxy: None,
                 }
             }
             (Env::Production, Replica::ReadOnlyMirror) => {
@@ -89,7 +88,6 @@ impl Default for Config {
                         &api_protocol,
                     ),
                     cdn: dotenv::var("S3_CDN").ok(),
-                    proxy: None,
                 }
             }
             // In Development mode, either running as a primary instance or a read-only mirror
@@ -109,7 +107,6 @@ impl Default for Config {
                             &api_protocol,
                         ),
                         cdn: dotenv::var("S3_CDN").ok(),
-                        proxy: None,
                     }
                 } else {
                     // If we don't set the `S3_BUCKET` variable, we'll use a development-only

src/controllers/krate/metadata.rs

@@ -169,8 +169,7 @@ pub fn readme(req: &mut dyn Request) -> CargoResult<Response> {
         .app()
         .config
         .uploader
-        .readme_location(crate_name, version)
-        .ok_or_else(|| human("crate readme not found"))?;
+        .readme_location(crate_name, version);
 
     if req.wants_json() {
         #[derive(Serialize)]

src/controllers/version/downloads.rs

@@ -24,8 +24,7 @@ pub fn download(req: &mut dyn Request) -> CargoResult<Response> {
         .app()
         .config
         .uploader
-        .crate_location(crate_name, version)
-        .ok_or_else(|| human("crate files not found"))?;
+        .crate_location(crate_name, version);
 
     if req.wants_json() {
         #[derive(Serialize)]

src/github.rs

@@ -20,8 +20,7 @@ where
     let url = format!("{}://api.github.com{}", app.config.api_protocol, url);
     info!("GITHUB HTTP: {}", url);
 
-    let client = app.http_client()?;
-    client
+    app.http_client()
         .get(&url)
         .header(header::ACCEPT, "application/vnd.github.v3+json")
         .header(

src/tests/all.rs

@@ -29,6 +29,7 @@ use std::{
 use conduit::Request;
 use conduit_test::MockRequest;
 use diesel::prelude::*;
+use reqwest::{Client, Proxy};
 use url::Url;
 
 macro_rules! t {
@@ -113,6 +114,13 @@ fn app() -> (
     conduit_middleware::MiddlewareBuilder,
 ) {
     let (proxy, bomb) = record::proxy();
+    let (app, handler) = simple_app(Some(proxy));
+    (bomb, app, handler)
+}
+
+fn simple_app(proxy: Option<String>) -> (Arc<App>, conduit_middleware::MiddlewareBuilder) {
+    git::init();
+
     let uploader = Uploader::S3 {
         bucket: s3::Bucket::new(
             String::from("alexcrichton-test"),
@@ -123,16 +131,9 @@ fn app() -> (
             // sniff/record it, but everywhere else we use https
             "http",
         ),
-        proxy: Some(proxy),
         cdn: None,
     };
 
-    let (app, handler) = simple_app(uploader);
-    (bomb, app, handler)
-}
-
-fn simple_app(uploader: Uploader) -> (Arc<App>, conduit_middleware::MiddlewareBuilder) {
-    git::init();
     let config = Config {
         uploader,
         session_key: "test this has to be over 32 bytes long".to_string(),
@@ -149,7 +150,17 @@ fn simple_app(uploader: Uploader) -> (Arc<App>, conduit_middleware::MiddlewareBu
         // sniff/record it, but everywhere else we use https
         api_protocol: String::from("http"),
     };
-    let app = App::new(&config);
+
+    let client = if let Some(proxy) = proxy {
+        let mut builder = Client::builder();
+        builder = builder
+            .proxy(Proxy::all(&proxy).expect("Unable to configure proxy with the provided URL"));
+        Some(builder.build().expect("TLS backend cannot be initialized"))
+    } else {
+        None
+    };
+
+    let app = App::new(&config, client);
     t!(t!(app.diesel_database.get()).begin_test_transaction());
     let app = Arc::new(app);
     let handler = cargo_registry::build_handler(Arc::clone(&app));

src/tests/util.rs

@@ -26,7 +26,6 @@ use crate::{
 use cargo_registry::{
     middleware::current_user::AuthenticationSource,
     models::{ApiToken, User},
-    uploaders::Uploader,
     App,
 };
 use diesel::PgConnection;
@@ -48,7 +47,7 @@ pub struct TestApp(Rc<TestAppInner>);
 impl TestApp {
     /// Initialize an application with an `Uploader` that panics
     pub fn init() -> TestAppBuilder {
-        let (app, middle) = crate::simple_app(Uploader::Panic);
+        let (app, middle) = crate::simple_app(None);
         let inner = Rc::new(TestAppInner {
             app,
             _bomb: None,