Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Disable connection pool to fix dist server feature #1612

Merged
merged 12 commits into from
Feb 22, 2023
4 changes: 3 additions & 1 deletion .github/actions/artifact_failure/action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
8 changes: 4 additions & 4 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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 }}
Expand Down Expand Up @@ -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: ""
Expand Down
6 changes: 3 additions & 3 deletions src/bin/sccache-dist/token_check.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -97,7 +97,7 @@ impl MozillaCheck {
pub fn new(required_groups: Vec<String>) -> Self {
Self {
auth_cache: Mutex::new(HashMap::new()),
client: reqwest::blocking::Client::new(),
client: new_reqwest_blocking_client(),
required_groups,
}
}
Expand Down Expand Up @@ -266,7 +266,7 @@ impl ProxyTokenCheck {
let maybe_auth_cache: Option<Mutex<(HashMap<String, Instant>, 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,
}
Expand Down
1 change: 0 additions & 1 deletion src/compiler/compiler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down
3 changes: 2 additions & 1 deletion src/dist/client_auth.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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!(
Expand Down
23 changes: 13 additions & 10 deletions src/dist/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -64,9 +61,12 @@ mod common {
Ok(self.bytes(bytes))
}
fn bytes(self, bytes: Vec<u8>) -> 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)
Expand All @@ -79,7 +79,7 @@ mod common {
) -> Result<T> {
// 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?;
Expand Down Expand Up @@ -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};
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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
Expand Down
17 changes: 17 additions & 0 deletions src/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -507,6 +507,23 @@ pub fn daemonize() -> Result<()> {
Ok(())
}

/// Disable connection pool to avoid broken connection between runtime
Xuanwo marked this conversation as resolved.
Show resolved Hide resolved
///
/// # 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;
Expand Down
2 changes: 1 addition & 1 deletion tests/dist.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()),
];
Expand Down
9 changes: 5 additions & 4 deletions tests/harness/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down Expand Up @@ -264,7 +265,7 @@ impl DistSystem {
"-e",
"SCCACHE_NO_DAEMON=1",
"-e",
"SCCACHE_LOG=sccache=trace",
"SCCACHE_LOG=debug",
"-e",
"RUST_BACKTRACE=1",
"--network",
Expand Down Expand Up @@ -332,7 +333,7 @@ impl DistSystem {
"--name",
&server_name,
"-e",
"SCCACHE_LOG=sccache=trace",
"SCCACHE_LOG=debug",
"-e",
"RUST_BACKTRACE=1",
"--network",
Expand Down