From 96c8559ac44a554194755a1ccc7fb57c6982d33e Mon Sep 17 00:00:00 2001 From: Justin Geibel Date: Tue, 19 Mar 2019 01:30:09 -0400 Subject: [PATCH] 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. --- src/app.rs | 37 +++++++++++++-------- src/bin/render-readmes.rs | 24 ++++++++------ src/bin/server.rs | 5 ++- src/config.rs | 3 -- src/controllers/krate/metadata.rs | 3 +- src/controllers/version/downloads.rs | 3 +- src/github.rs | 3 +- src/tests/all.rs | 27 +++++++++++----- src/tests/util.rs | 3 +- src/uploaders.rs | 48 ++++++---------------------- 10 files changed, 75 insertions(+), 81 deletions(-) diff --git a/src/app.rs b/src/app.rs index 6d56a488cae..132858d2da5 100644 --- a/src/app.rs +++ b/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, } 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) -> 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 { - 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()`.") } } diff --git a/src/bin/render-readmes.rs b/src/bin/render-readmes.rs index ea636401a33..e3fa976e544 100644 --- a/src/bin/render-readmes.rs +++ b/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,9 +120,10 @@ 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; } @@ -127,12 +131,7 @@ fn main() { 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 { +fn get_readme( + config: &Config, + client: &Client, + version: &Version, + krate_name: &str, +) -> Option { 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!( diff --git a/src/bin/server.rs b/src/bin/server.rs index 8d97081f657..954625be6dc 100644 --- a/src/bin/server.rs +++ b/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 diff --git a/src/config.rs b/src/config.rs index 31aaf7ec527..44b030370aa 100644 --- a/src/config.rs +++ b/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 diff --git a/src/controllers/krate/metadata.rs b/src/controllers/krate/metadata.rs index 63eeb9000cd..726d377a8d6 100644 --- a/src/controllers/krate/metadata.rs +++ b/src/controllers/krate/metadata.rs @@ -169,8 +169,7 @@ pub fn readme(req: &mut dyn Request) -> CargoResult { .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)] diff --git a/src/controllers/version/downloads.rs b/src/controllers/version/downloads.rs index df0053c4e22..c7db7749610 100644 --- a/src/controllers/version/downloads.rs +++ b/src/controllers/version/downloads.rs @@ -24,8 +24,7 @@ pub fn download(req: &mut dyn Request) -> CargoResult { .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)] diff --git a/src/github.rs b/src/github.rs index 731010bf2a7..f5b1a877407 100644 --- a/src/github.rs +++ b/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( diff --git a/src/tests/all.rs b/src/tests/all.rs index 35dea7a5aff..d5f4129aa7b 100644 --- a/src/tests/all.rs +++ b/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) -> (Arc, 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, 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, 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)); diff --git a/src/tests/util.rs b/src/tests/util.rs index 333fc453190..623b8474e77 100644 --- a/src/tests/util.rs +++ b/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); 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, diff --git a/src/uploaders.rs b/src/uploaders.rs index 0dbfe22e079..80b98519cf2 100644 --- a/src/uploaders.rs +++ b/src/uploaders.rs @@ -14,10 +14,6 @@ use crate::app::App; use crate::middleware::app::RequestApp; use crate::models::Crate; -fn require_test_app_with_proxy() -> ! { - panic!("No uploader is configured. In tests, use `TestApp::with_proxy()`."); -} - #[derive(Clone, Debug)] pub enum Uploader { /// For production usage, uploads and redirects to s3. @@ -25,32 +21,18 @@ pub enum Uploader { S3 { bucket: s3::Bucket, cdn: Option, - proxy: Option, }, /// For development usage only: "uploads" crate files to `dist` and serves them /// from there as well to enable local publishing and download Local, - - /// For tests using `TestApp::init()` - /// Attempts to get an outgoing HTTP handle will panic. - Panic, } impl Uploader { - pub fn proxy(&self) -> Option<&str> { - match *self { - Uploader::S3 { ref proxy, .. } => proxy.as_ref().map(String::as_str), - Uploader::Local => None, - Uploader::Panic => require_test_app_with_proxy(), - } - } - /// Returns the URL of an uploaded crate's version archive. /// /// The function doesn't check for the existence of the file. - /// It returns `None` if the current `Uploader` is `Panic`. - pub fn crate_location(&self, crate_name: &str, version: &str) -> Option { + pub fn crate_location(&self, crate_name: &str, version: &str) -> String { match *self { Uploader::S3 { ref bucket, @@ -62,18 +44,16 @@ impl Uploader { None => bucket.host(), }; let path = Uploader::crate_path(crate_name, version); - Some(format!("https://{}/{}", host, path)) + format!("https://{}/{}", host, path) } - Uploader::Local => Some(format!("/{}", Uploader::crate_path(crate_name, version))), - Uploader::Panic => require_test_app_with_proxy(), + Uploader::Local => format!("/{}", Uploader::crate_path(crate_name, version)), } } /// Returns the URL of an uploaded crate's version readme. /// /// The function doesn't check for the existence of the file. - /// It returns `None` if the current `Uploader` is `Panic`. - pub fn readme_location(&self, crate_name: &str, version: &str) -> Option { + pub fn readme_location(&self, crate_name: &str, version: &str) -> String { match *self { Uploader::S3 { ref bucket, @@ -85,10 +65,9 @@ impl Uploader { None => bucket.host(), }; let path = Uploader::readme_path(crate_name, version); - Some(format!("https://{}/{}", host, path)) + format!("https://{}/{}", host, path) } - Uploader::Local => Some(format!("/{}", Uploader::readme_path(crate_name, version))), - Uploader::Panic => require_test_app_with_proxy(), + Uploader::Local => format!("/{}", Uploader::readme_path(crate_name, version)), } } @@ -103,7 +82,7 @@ impl Uploader { format!("readmes/{}/{}-{}.html", name, name, version) } - /// Uploads a file using the configured uploader (either `S3`, `Local` or `Panic`). + /// Uploads a file using the configured uploader (either `S3`, `Local`). /// /// It returns a a tuple containing the path of the uploaded file /// and its checksum. @@ -130,7 +109,6 @@ impl Uploader { file.write_all(&body)?; Ok((filename.to_str().map(String::from), hash)) } - Uploader::Panic => require_test_app_with_proxy(), } } @@ -150,7 +128,7 @@ impl Uploader { let mut body = Vec::new(); LimitErrorReader::new(req.body(), maximums.max_upload_size).read_to_end(&mut body)?; verify_tarball(krate, vers, &body, maximums.max_unpack_size)?; - self.upload(&app.http_client()?, &path, body, "application/x-tar")? + self.upload(app.http_client(), &path, body, "application/x-tar")? }; // We create the bomb for the crate file before uploading the readme so that if the // readme upload fails, the uploaded crate file is automatically deleted. @@ -160,12 +138,7 @@ impl Uploader { }; let (readme_path, _) = if let Some(rendered) = readme { let path = Uploader::readme_path(&krate.name, &vers.to_string()); - self.upload( - &app.http_client()?, - &path, - rendered.into_bytes(), - "text/html", - )? + self.upload(app.http_client(), &path, rendered.into_bytes(), "text/html")? } else { (None, vec![]) }; @@ -183,14 +156,13 @@ impl Uploader { fn delete(&self, app: &Arc, path: &str) -> CargoResult<()> { match *self { Uploader::S3 { ref bucket, .. } => { - bucket.delete(&app.http_client()?, path)?; + bucket.delete(app.http_client(), path)?; Ok(()) } Uploader::Local => { fs::remove_file(path)?; Ok(()) } - Uploader::Panic => require_test_app_with_proxy(), } } }