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 too many 429 response #1231

Merged
merged 1 commit into from
Aug 8, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 0 additions & 25 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions crates/bin/src/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::{
env,
ffi::OsString,
fmt,
num::{NonZeroU64, ParseIntError},
num::{NonZeroU16, NonZeroU64, ParseIntError},
path::PathBuf,
str::FromStr,
};
Expand Down Expand Up @@ -348,7 +348,7 @@ impl From<TLSVersion> for remote::TLSVersion {

#[derive(Copy, Clone, Debug)]
pub struct RateLimit {
pub duration: NonZeroU64,
pub duration: NonZeroU16,
pub request_count: NonZeroU64,
}

Expand Down Expand Up @@ -379,7 +379,7 @@ impl FromStr for RateLimit {
impl Default for RateLimit {
fn default() -> Self {
Self {
duration: NonZeroU64::new(10).unwrap(),
duration: NonZeroU16::new(10).unwrap(),
request_count: NonZeroU64::new(1).unwrap(),
}
}
Expand Down
3 changes: 1 addition & 2 deletions crates/bin/src/entry.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ use std::{
future::Future,
path::{Path, PathBuf},
sync::Arc,
time::Duration,
};

use binstalk::{
Expand Down Expand Up @@ -100,7 +99,7 @@ pub fn install_crates(
let client = Client::new(
concat!(env!("CARGO_PKG_NAME"), "/", env!("CARGO_PKG_VERSION")),
args.min_tls_version.map(|v| v.into()),
Duration::from_millis(rate_limit.duration.get()),
rate_limit.duration,
rate_limit.request_count,
read_root_certs(
args.root_certificates,
Expand Down
1 change: 0 additions & 1 deletion crates/binstalk-downloader/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ thiserror = "1.0.40"
tokio = { version = "1.28.2", features = ["macros", "rt-multi-thread", "sync", "time", "fs"], default-features = false }
tokio-tar = "0.3.0"
tokio-util = { version = "0.7.8", features = ["io"] }
tower = { version = "0.4.13", features = ["limit", "util"] }
tracing = "0.1.37"
# trust-dns-resolver must be kept in sync with the version reqwest uses
trust-dns-resolver = { version = "0.22.0", optional = true, default-features = false, features = ["dnssec-ring"] }
Expand Down
3 changes: 2 additions & 1 deletion crates/binstalk-downloader/src/download.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ mod test {
use std::{
collections::{HashMap, HashSet},
ffi::OsStr,
num::NonZeroU16,
};
use tempfile::tempdir;

Expand All @@ -231,7 +232,7 @@ mod test {
let client = crate::remote::Client::new(
concat!(env!("CARGO_PKG_NAME"), "/", env!("CARGO_PKG_VERSION")),
None,
std::time::Duration::from_millis(10),
NonZeroU16::new(10).unwrap(),
1.try_into().unwrap(),
[],
)
Expand Down
6 changes: 3 additions & 3 deletions crates/binstalk-downloader/src/gh_api_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ mod request;
pub use request::{GhApiContextError, GhApiError, GhGraphQLErrors};

/// default retry duration if x-ratelimit-reset is not found in response header
const DEFAULT_RETRY_DURATION: Duration = Duration::from_secs(5 * 60);
const DEFAULT_RETRY_DURATION: Duration = Duration::from_secs(10 * 60);

fn percent_encode_http_url_path(path: &str) -> PercentEncode<'_> {
/// https://url.spec.whatwg.org/#fragment-percent-encode-set
Expand Down Expand Up @@ -289,7 +289,7 @@ pub enum HasReleaseArtifact {
mod test {
use super::*;
use compact_str::{CompactString, ToCompactString};
use std::env;
use std::{env, num::NonZeroU16};

mod cargo_binstall_v0_20_1 {
use super::{CompactString, GhRelease};
Expand Down Expand Up @@ -383,7 +383,7 @@ mod test {
let client = remote::Client::new(
concat!(env!("CARGO_PKG_NAME"), "/", env!("CARGO_PKG_VERSION")),
None,
Duration::from_millis(10),
NonZeroU16::new(10).unwrap(),
1.try_into().unwrap(),
[],
)
Expand Down
25 changes: 12 additions & 13 deletions crates/binstalk-downloader/src/remote.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use std::{
num::{NonZeroU64, NonZeroU8},
num::{NonZeroU16, NonZeroU64, NonZeroU8},
ops::ControlFlow,
sync::Arc,
time::{Duration, SystemTime},
Expand All @@ -13,7 +13,6 @@ use reqwest::{
Request,
};
use thiserror::Error as ThisError;
use tower::{limit::rate::RateLimit, Service, ServiceBuilder, ServiceExt};
use tracing::{debug, info};

pub use reqwest::{header, Error as ReqwestError, Method, StatusCode};
Expand Down Expand Up @@ -73,30 +72,32 @@ impl HttpError {
#[derive(Debug)]
struct Inner {
client: reqwest::Client,
service: DelayRequest<RateLimit<reqwest::Client>>,
service: DelayRequest,
}

#[derive(Clone, Debug)]
pub struct Client(Arc<Inner>);

#[cfg_attr(not(feature = "__tls"), allow(unused_variables, unused_mut))]
impl Client {
/// * `per` - must not be 0.
/// * `per_millis` - The duration (in millisecond) for which at most
/// `num_request` can be sent, itcould be increased if rate-limit
/// happens.
/// * `num_request` - maximum number of requests to be processed for
/// each `per` duration.
///
/// The Client created would use at least tls 1.2
pub fn new(
user_agent: impl AsRef<str>,
min_tls: Option<TLSVersion>,
per: Duration,
per_millis: NonZeroU16,
num_request: NonZeroU64,
certificates: impl IntoIterator<Item = Certificate>,
) -> Result<Self, Error> {
fn inner(
user_agent: &str,
min_tls: Option<TLSVersion>,
per: Duration,
per_millis: NonZeroU16,
num_request: NonZeroU64,
certificates: &mut dyn Iterator<Item = Certificate>,
) -> Result<Client, Error> {
Expand All @@ -123,17 +124,17 @@ impl Client {
Ok(Client(Arc::new(Inner {
client: client.clone(),
service: DelayRequest::new(
ServiceBuilder::new()
.rate_limit(num_request.get(), per)
.service(client),
num_request,
Duration::from_millis(per_millis.get() as u64),
client,
),
})))
}

inner(
user_agent.as_ref(),
min_tls,
per,
per_millis,
num_request,
&mut certificates.into_iter(),
)
Expand All @@ -159,9 +160,7 @@ impl Client {
url: &Url,
) -> Result<ControlFlow<reqwest::Response, Result<reqwest::Response, ReqwestError>>, ReqwestError>
{
let future = (&self.0.service).ready().await?.call(request);

let response = match future.await {
let response = match self.0.service.call(request).await {
Err(err) if err.is_timeout() || err.is_connect() => {
let duration = RETRY_DURATION_FOR_TIMEOUT;

Expand Down
Loading