From 520a30b6814fc5afbc7bd2fa1d6bd2588ea58058 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Thu, 23 Feb 2023 00:06:51 +0800 Subject: [PATCH] fix: Disable connection pool to fix dist server feature (#1612) Signed-off-by: Xuanwo Co-authored-by: Sylvestre Ledru Co-authored-by: Bernhard Schuster --- .github/actions/artifact_failure/action.yml | 4 +++- .github/workflows/ci.yml | 8 +++---- src/bin/sccache-dist/token_check.rs | 6 +++--- src/compiler/compiler.rs | 1 - src/dist/client_auth.rs | 3 ++- src/dist/http.rs | 23 ++++++++++++--------- src/util.rs | 17 +++++++++++++++ tests/dist.rs | 2 +- tests/harness/mod.rs | 9 ++++---- 9 files changed, 48 insertions(+), 25 deletions(-) diff --git a/.github/actions/artifact_failure/action.yml b/.github/actions/artifact_failure/action.yml index febe873ce..d83c76229 100644 --- a/.github/actions/artifact_failure/action.yml +++ b/.github/actions/artifact_failure/action.yml @@ -25,4 +25,6 @@ runs: - uses: actions/upload-artifact@v3 with: name: ${{ inputs.name }} - path: target/failure-${{ inputs.name }}.tar.gz + path: | + target/failure-${{ inputs.name }}.tar.gz + /tmp/sccache_*.txt diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 336e85cf7..d20203960 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -82,10 +82,10 @@ jobs: if: ${{ matrix.os == 'ubuntu-20.04' || matrix.os == 'ubuntu-22.04' }} - name: Build tests - run: cargo test --no-run --locked --all-targets --verbose ${{ matrix.extra_args }} + run: cargo test --no-run --locked --all-targets ${{ matrix.extra_args }} - name: Run tests - run: cargo test --locked --all-targets --verbose ${{ matrix.extra_args }} + run: cargo test --locked --all-targets ${{ matrix.extra_args }} - name: Upload failure if: failure() @@ -150,7 +150,7 @@ jobs: if: ${{ matrix.target == 'aarch64-unknown-linux-musl' }} - name: Build - run: cargo build --locked --release --verbose --bin ${{ matrix.binary || 'sccache' }} --target ${{ matrix.target }} --features=openssl/vendored ${{ matrix.extra_args }} + run: cargo build --locked --release --bin ${{ matrix.binary || 'sccache' }} --target ${{ matrix.target }} --features=openssl/vendored ${{ matrix.extra_args }} env: CARGO_TARGET_AARCH64_UNKNOWN_LINUX_MUSL_LINKER: aarch64-linux-musl-gcc MACOSX_DEPLOYMENT_TARGET: ${{ matrix.macosx_deployment_target }} @@ -211,7 +211,7 @@ jobs: run: cargo install grcov - name: Execute tests - run: cargo test --no-fail-fast --locked --all-targets --verbose ${{ matrix.extra_args }} + run: cargo test --no-fail-fast --locked --all-targets ${{ matrix.extra_args }} env: CARGO_INCREMENTAL: "0" RUSTC_WRAPPER: "" diff --git a/src/bin/sccache-dist/token_check.rs b/src/bin/sccache-dist/token_check.rs index b22d61932..0f1c7b65b 100644 --- a/src/bin/sccache-dist/token_check.rs +++ b/src/bin/sccache-dist/token_check.rs @@ -1,7 +1,7 @@ use crate::jwt; use anyhow::{bail, Context, Result}; use sccache::dist::http::{ClientAuthCheck, ClientVisibleMsg}; -use sccache::util::BASE64_URL_SAFE_ENGINE; +use sccache::util::{new_reqwest_blocking_client, BASE64_URL_SAFE_ENGINE}; use std::collections::HashMap; use std::result::Result as StdResult; use std::sync::Mutex; @@ -97,7 +97,7 @@ impl MozillaCheck { pub fn new(required_groups: Vec) -> Self { Self { auth_cache: Mutex::new(HashMap::new()), - client: reqwest::blocking::Client::new(), + client: new_reqwest_blocking_client(), required_groups, } } @@ -266,7 +266,7 @@ impl ProxyTokenCheck { let maybe_auth_cache: Option, Duration)>> = cache_secs.map(|secs| Mutex::new((HashMap::new(), Duration::from_secs(secs)))); Self { - client: reqwest::blocking::Client::new(), + client: new_reqwest_blocking_client(), maybe_auth_cache, url, } diff --git a/src/compiler/compiler.rs b/src/compiler/compiler.rs index c515d3313..8fe962c29 100644 --- a/src/compiler/compiler.rs +++ b/src/compiler/compiler.rs @@ -31,7 +31,6 @@ use crate::mock_command::{exit_status, CommandChild, CommandCreatorSync, RunComm use crate::util::{fmt_duration_as_secs, ref_env, run_input_output}; use filetime::FileTime; use fs::File; -#[cfg(feature = "dist-client")] use fs_err as fs; use std::borrow::Cow; use std::ffi::{OsStr, OsString}; diff --git a/src/dist/client_auth.rs b/src/dist/client_auth.rs index 685b5c423..099171361 100644 --- a/src/dist/client_auth.rs +++ b/src/dist/client_auth.rs @@ -84,6 +84,7 @@ mod code_grant_pkce { html_response, json_response, query_pairs, MIN_TOKEN_VALIDITY, MIN_TOKEN_VALIDITY_WARNING, REDIRECT_WITH_AUTH_JSON, }; + use crate::util::new_reqwest_blocking_client; use crate::util::BASE64_URL_SAFE_ENGINE; use futures::channel::oneshot; use hyper::{Body, Method, Request, Response, StatusCode}; @@ -242,7 +243,7 @@ mod code_grant_pkce { grant_type: GRANT_TYPE_PARAM_VALUE, redirect_uri, }; - let client = reqwest::blocking::Client::new(); + let client = new_reqwest_blocking_client(); let res = client.post(token_url).json(&token_request).send()?; if !res.status().is_success() { bail!( diff --git a/src/dist/http.rs b/src/dist/http.rs index 6bb782976..d854b5595 100644 --- a/src/dist/http.rs +++ b/src/dist/http.rs @@ -22,9 +22,6 @@ pub use self::server::{ mod common { use http::header; - use http::header::CONNECTION; - use http::header::CONTENT_LENGTH; - use http::header::CONTENT_TYPE; #[cfg(feature = "dist-server")] use std::collections::HashMap; use std::fmt; @@ -64,9 +61,12 @@ mod common { Ok(self.bytes(bytes)) } fn bytes(self, bytes: Vec) -> Self { - self.header(CONTENT_TYPE, mime::APPLICATION_OCTET_STREAM.to_string()) - .header(CONTENT_LENGTH, bytes.len()) - .body(bytes) + self.header( + header::CONTENT_TYPE, + mime::APPLICATION_OCTET_STREAM.to_string(), + ) + .header(header::CONTENT_LENGTH, bytes.len()) + .body(bytes) } fn bearer_auth(self, token: String) -> Self { self.bearer_auth(token) @@ -79,7 +79,7 @@ mod common { ) -> Result { // Work around tiny_http issue #151 by disabling HTTP pipeline with // `Connection: close`. - let res = req.header(CONNECTION, "close").send().await?; + let res = req.header(header::CONNECTION, "close").send().await?; let status = res.status(); let bytes = res.bytes().await?; @@ -250,6 +250,7 @@ pub mod urls { #[cfg(feature = "dist-server")] mod server { use crate::jwt; + use crate::util::new_reqwest_blocking_client; use byteorder::{BigEndian, ReadBytesExt}; use flate2::read::ZlibDecoder as ZlibReadDecoder; use rand::{rngs::OsRng, RngCore}; @@ -685,7 +686,7 @@ mod server { check_server_auth, } = self; let requester = SchedulerRequester { - client: Mutex::new(reqwest::blocking::Client::new()), + client: Mutex::new(new_reqwest_blocking_client()), }; macro_rules! check_server_auth_or_err { @@ -939,14 +940,14 @@ mod server { let job_authorizer = JWTJobAuthorizer::new(jwt_key); let heartbeat_url = urls::scheduler_heartbeat_server(&scheduler_url); let requester = ServerRequester { - client: reqwest::blocking::Client::new(), + client: new_reqwest_blocking_client(), scheduler_url, scheduler_auth: scheduler_auth.clone(), }; // TODO: detect if this panics thread::spawn(move || { - let client = reqwest::blocking::Client::new(); + let client = new_reqwest_blocking_client(); loop { trace!("Performing heartbeat"); match bincode_req( @@ -1152,6 +1153,8 @@ mod client { let timeout = Duration::new(REQUEST_TIMEOUT_SECS, 0); let new_client_async = client_async_builder .timeout(timeout) + // Disable keep-alive + .pool_max_idle_per_host(0) .build() .context("failed to create an async HTTP client")?; // Use the updated certificates diff --git a/src/util.rs b/src/util.rs index 8d85f541d..e4d04f7f9 100644 --- a/src/util.rs +++ b/src/util.rs @@ -507,6 +507,23 @@ pub fn daemonize() -> Result<()> { Ok(()) } +/// Disable connection pool to avoid broken connection between runtime +/// +/// # TODO +/// +/// We should refactor sccache current model to make sure that we only have +/// one tokio runtime and keep reqwest alive inside it. +/// +/// --- +/// +/// More details could be found at https://github.com/mozilla/sccache/pull/1563 +pub fn new_reqwest_blocking_client() -> reqwest::blocking::Client { + reqwest::blocking::Client::builder() + .pool_max_idle_per_host(0) + .build() + .expect("http client must build with success") +} + #[cfg(test)] mod tests { use super::OsStrExt; diff --git a/tests/dist.rs b/tests/dist.rs index 516d0a179..4885e0451 100644 --- a/tests/dist.rs +++ b/tests/dist.rs @@ -25,7 +25,7 @@ mod harness; fn basic_compile(tmpdir: &Path, sccache_cfg_path: &Path, sccache_cached_cfg_path: &Path) { let envs: Vec<(_, &OsStr)> = vec![ ("RUST_BACKTRACE", "1".as_ref()), - ("SCCACHE_LOG", "trace".as_ref()), + ("SCCACHE_LOG", "debug".as_ref()), ("SCCACHE_CONF", sccache_cfg_path.as_ref()), ("SCCACHE_CACHED_CONF", sccache_cached_cfg_path.as_ref()), ]; diff --git a/tests/harness/mod.rs b/tests/harness/mod.rs index 3bb84660b..1f4b55f6d 100644 --- a/tests/harness/mod.rs +++ b/tests/harness/mod.rs @@ -46,14 +46,15 @@ pub fn start_local_daemon(cfg_path: &Path, cached_cfg_path: &Path) { let _ = sccache_command() .arg("--start-server") // Uncomment following lines to debug locally. - // .env("SCCACHE_LOG", "debug") - // .env("SCCACHE_ERROR_LOG", "/tmp/sccache_log.txt") + .env("SCCACHE_LOG", "debug") + .env("SCCACHE_ERROR_LOG", "/tmp/sccache_local_daemon.txt") .env("SCCACHE_CONF", cfg_path) .env("SCCACHE_CACHED_CONF", cached_cfg_path) .status() .unwrap() .success(); } + pub fn stop_local_daemon() { trace!("sccache --stop-server"); drop( @@ -264,7 +265,7 @@ impl DistSystem { "-e", "SCCACHE_NO_DAEMON=1", "-e", - "SCCACHE_LOG=sccache=trace", + "SCCACHE_LOG=debug", "-e", "RUST_BACKTRACE=1", "--network", @@ -332,7 +333,7 @@ impl DistSystem { "--name", &server_name, "-e", - "SCCACHE_LOG=sccache=trace", + "SCCACHE_LOG=debug", "-e", "RUST_BACKTRACE=1", "--network",