Initialize a single reqwest::Client for the server process

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,