Skip to content

Commit

Permalink
feat: Replace garcon with backoff (#392)
Browse files Browse the repository at this point in the history
Implements SDK-824.

This additionally removes garcon from the public API; the backoff numbers dfx uses are good enough for anyone, and users who want a shorter timeout can use tokio's timeout function.
  • Loading branch information
adamspofford-dfinity authored Nov 19, 2022
1 parent 929695e commit 4968280
Show file tree
Hide file tree
Showing 17 changed files with 203 additions and 375 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

* Remove `garcon` from API. Callers can remove the dependency and any usages of it; all waiting functions no longer take a waiter parameter.

## [0.22.0] - 2022-10-17

* Drop `disable_range_check` flag from certificate delegation checking.
Expand Down
25 changes: 12 additions & 13 deletions Cargo.lock

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

9 changes: 6 additions & 3 deletions ic-agent/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,16 @@ rust-version = "1.60.0"

[dependencies]
async-trait = "0.1.53"
backoff = "0.4.0"
base32 = "0.4.0"
base64 = "0.13.0"
byteorder = "1.3.2"
candid = "0.8.0"
garcon = { version = "0.2", features = ["async"] }
futures-util = "0.3.21"
hex = "0.4.0"
http = "0.2.6"
http-body = "0.4.5"
hyper-rustls = { version = "0.23.0", features = [ "webpki-roots", "http2" ] }
hyper-rustls = { version = "0.23.0", features = [ "webpki-roots", "http2" ], optional = true }
ic-verify-bls-signature = "0.1"
k256 = { version = "0.11", features = ["pem"] }
leb128 = "0.2.5"
Expand All @@ -38,10 +39,10 @@ serde_cbor = "0.11.2"
sha2 = "0.10"
simple_asn1 = "0.6.1"
thiserror = "1.0.30"
tokio = { version = "1.21.2", features = ["time"] }
url = "2.1.0"
pkcs8 = { version = "0.9", features = ["std"] }
sec1 = { version = "0.3", features = ["pem"] }
futures-util = "0.3.21"

[dependencies.hyper]
version = "0.14"
Expand All @@ -66,4 +67,6 @@ tokio = { version = "1.17.0", features = ["full"] }

[features]
default = ["pem", "reqwest"]
reqwest = ["dep:reqwest", "dep:hyper-rustls"]
hyper = ["dep:hyper", "dep:hyper-rustls"]
ic_ref_tests = ["default"] # Used to separate integration tests for ic-ref which need a server running.
4 changes: 0 additions & 4 deletions ic-agent/src/agent/agent_error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@ pub enum AgentError {
#[error("The request timed out.")]
TimeoutWaitingForResponse(),

/// The waiter was restarted without being started first.
#[error("The waiter was restarted without being started first.")]
WaiterRestartError(),

/// An error occurred when signing with the identity.
#[error("Identity had a signing error: {0}")]
SigningError(String),
Expand Down
62 changes: 22 additions & 40 deletions ic-agent/src/agent/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ pub mod signed;
pub mod status;
pub use agent_config::AgentConfig;
pub use agent_error::AgentError;
use backoff::{backoff::Backoff, ExponentialBackoffBuilder};
pub use builder::AgentBuilder;
pub use nonce::{NonceFactory, NonceGenerator};
pub use response::{Replied, RequestStatusResponse};
Expand All @@ -29,7 +30,6 @@ use crate::{
identity::Identity,
to_request_id, RequestId,
};
use garcon::Waiter;
use serde::Serialize;
use status::Status;

Expand Down Expand Up @@ -205,19 +205,14 @@ pub enum PollResult {
/// agent.fetch_root_key().await?;
/// let management_canister_id = Principal::from_text("aaaaa-aa")?;
///
/// let waiter = garcon::Delay::builder()
/// .throttle(std::time::Duration::from_millis(500))
/// .timeout(std::time::Duration::from_secs(60 * 5))
/// .build();
///
/// // Create a call to the management canister to create a new canister ID,
/// // and wait for a result.
/// // The effective canister id must belong to the canister ranges of the subnet at which the canister is created.
/// let effective_canister_id = Principal::from_text("sehci-oaaaa-aaaaa-aaaaa-c").unwrap();
/// let response = agent.update(&management_canister_id, "provisional_create_canister_with_cycles")
/// .with_effective_canister_id(effective_canister_id)
/// .with_arg(&Encode!(&Argument { amount: None })?)
/// .call_and_wait(waiter)
/// .call_and_wait()
/// .await?;
///
/// let result = Decode!(response.as_slice(), CreateCanisterResult)?;
Expand Down Expand Up @@ -548,13 +543,16 @@ impl Agent {
}

/// Call request_status on the RequestId in a loop and return the response as a byte vector.
pub async fn wait<W: Waiter>(
pub async fn wait(
&self,
request_id: RequestId,
effective_canister_id: Principal,
mut waiter: W,
) -> Result<Vec<u8>, AgentError> {
waiter.start();
let mut retry_policy = ExponentialBackoffBuilder::new()
.with_initial_interval(Duration::from_millis(200))
.with_max_interval(Duration::from_secs(1))

This comment has been minimized.

Copy link
@rumenov

rumenov Apr 27, 2023

Contributor

This is way too little. You will most certainly always a get a timeout error always when you send an update request.

1 second for receiving the state back most certainly is unsufficient

.with_multiplier(1.4)
.build();
let mut request_accepted = false;
loop {
match self.poll(&request_id, effective_canister_id).await? {
Expand All @@ -566,21 +564,19 @@ impl Agent {
// and we generally cannot know how long that will take.
// State transitions between Received and Processing may be
// instantaneous. Therefore, once we know the request is accepted,
// we should restart the waiter so the request does not time out.
// we should restart the backoff so the request does not time out.

waiter
.restart()
.map_err(|_| AgentError::WaiterRestartError())?;
retry_policy.reset();
request_accepted = true;
}
}
PollResult::Completed(result) => return Ok(result),
};

waiter
.async_wait()
.await
.map_err(|_| AgentError::TimeoutWaitingForResponse())?;
match retry_policy.next_backoff() {
Some(duration) => tokio::time::sleep(duration).await,
None => return Err(AgentError::TimeoutWaitingForResponse()),
}
}
}

Expand Down Expand Up @@ -1127,26 +1123,12 @@ impl Future for UpdateCall<'_> {
self.request_id.as_mut().poll(cx)
}
}
impl UpdateCall<'_> {
fn and_wait<'out, W>(
self,
waiter: W,
) -> Pin<Box<dyn core::future::Future<Output = Result<Vec<u8>, AgentError>> + Send + 'out>>
where
Self: 'out,
W: Waiter + 'out,
{
async fn run<W>(_self: UpdateCall<'_>, waiter: W) -> Result<Vec<u8>, AgentError>
where
W: Waiter,
{
let request_id = _self.request_id.await?;
_self
.agent
.wait(request_id, _self.effective_canister_id, waiter)
.await
}
Box::pin(run(self, waiter))
impl<'a> UpdateCall<'a> {
async fn and_wait(self) -> Result<Vec<u8>, AgentError> {
let request_id = self.request_id.await?;
self.agent
.wait(request_id, self.effective_canister_id)
.await
}
}
/// An Update Request Builder.
Expand Down Expand Up @@ -1227,8 +1209,8 @@ impl<'agent> UpdateBuilder<'agent> {

/// Make an update call. This will call request_status on the RequestId in a loop and return
/// the response as a byte vector.
pub async fn call_and_wait<W: Waiter>(&self, waiter: W) -> Result<Vec<u8>, AgentError> {
self.call().and_wait(waiter).await
pub async fn call_and_wait(&self) -> Result<Vec<u8>, AgentError> {
self.call().and_wait().await
}

/// Make an update call. This will return a RequestId.
Expand Down
7 changes: 1 addition & 6 deletions ic-agent/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,19 +68,14 @@
//! agent.fetch_root_key().await?;
//! let management_canister_id = Principal::from_text("aaaaa-aa")?;
//!
//! let waiter = garcon::Delay::builder()
//! .throttle(std::time::Duration::from_millis(500))
//! .timeout(std::time::Duration::from_secs(60 * 5))
//! .build();
//!
//! // Create a call to the management canister to create a new canister ID,
//! // and wait for a result.
//! // The effective canister id must belong to the canister ranges of the subnet at which the canister is created.
//! let effective_canister_id = Principal::from_text("sehci-oaaaa-aaaaa-aaaaa-c").unwrap();
//! let response = agent.update(&management_canister_id, "provisional_create_canister_with_cycles")
//! .with_effective_canister_id(effective_canister_id)
//! .with_arg(&Encode!(&Argument { amount: None})?)
//! .call_and_wait(waiter)
//! .call_and_wait()
//! .await?;
//!
//! let result = Decode!(response.as_slice(), CreateCanisterResult)?;
Expand Down
1 change: 0 additions & 1 deletion ic-utils/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ rust-version = "1.60.0"
[dependencies]
async-trait = "0.1.40"
candid = "0.8.0"
garcon = { version = "0.2", features = ["async"] }
ic-agent = { path = "../ic-agent", version = "0.22", default-features = false }
serde = "1.0.115"
serde_bytes = "0.11"
Expand Down
Loading

0 comments on commit 4968280

Please sign in to comment.