diff --git a/Cargo.lock b/Cargo.lock index fdc49fb1ea51..62f72cda37b8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6295,7 +6295,6 @@ dependencies = [ name = "ic-artifact-downloader" version = "0.9.0" dependencies = [ - "anyhow", "axum 0.8.4", "backoff", "bytes", @@ -7686,7 +7685,6 @@ dependencies = [ name = "ic-consensus-manager" version = "0.9.0" dependencies = [ - "anyhow", "axum 0.8.4", "backoff", "bytes", @@ -9417,7 +9415,6 @@ dependencies = [ name = "ic-http-endpoints-async-utils" version = "0.9.0" dependencies = [ - "anyhow", "async-stream", "byte-unit", "bytes", @@ -10598,7 +10595,6 @@ dependencies = [ name = "ic-memory-transport" version = "0.9.0" dependencies = [ - "anyhow", "async-trait", "axum 0.8.4", "bytes", @@ -11915,7 +11911,6 @@ dependencies = [ name = "ic-p2p-test-utils" version = "0.9.0" dependencies = [ - "anyhow", "async-trait", "axum 0.8.4", "bytes", @@ -12091,7 +12086,6 @@ dependencies = [ name = "ic-quic-transport" version = "0.9.0" dependencies = [ - "anyhow", "assert_matches", "async-trait", "axum 0.8.4", diff --git a/rs/http_endpoints/async_utils/BUILD.bazel b/rs/http_endpoints/async_utils/BUILD.bazel index d5d3564e5a4e..fdcf10a0c11d 100644 --- a/rs/http_endpoints/async_utils/BUILD.bazel +++ b/rs/http_endpoints/async_utils/BUILD.bazel @@ -10,7 +10,6 @@ rust_library( deps = [ # Keep sorted. "//rs/monitoring/logger", - "@crate_index//:anyhow", "@crate_index//:async-stream", "@crate_index//:byte-unit", "@crate_index//:bytes", diff --git a/rs/http_endpoints/async_utils/Cargo.toml b/rs/http_endpoints/async_utils/Cargo.toml index 7f6369989866..1b1726d55da2 100644 --- a/rs/http_endpoints/async_utils/Cargo.toml +++ b/rs/http_endpoints/async_utils/Cargo.toml @@ -7,7 +7,6 @@ description.workspace = true documentation.workspace = true [dependencies] -anyhow = { workspace = true } async-stream = { workspace = true } byte-unit = "4.0.14" bytes = { workspace = true } diff --git a/rs/p2p/artifact_downloader/BUILD.bazel b/rs/p2p/artifact_downloader/BUILD.bazel index 0c1714af4ff4..beb320bf2442 100644 --- a/rs/p2p/artifact_downloader/BUILD.bazel +++ b/rs/p2p/artifact_downloader/BUILD.bazel @@ -32,7 +32,6 @@ DEV_DEPENDENCIES = [ "//rs/test_utilities/consensus", "//rs/test_utilities/types", "//rs/types/types_test_utils", - "@crate_index//:anyhow", "@crate_index//:futures", "@crate_index//:http-body-util", "@crate_index//:mockall", diff --git a/rs/p2p/artifact_downloader/Cargo.toml b/rs/p2p/artifact_downloader/Cargo.toml index df3119834326..50f7a86c17e3 100644 --- a/rs/p2p/artifact_downloader/Cargo.toml +++ b/rs/p2p/artifact_downloader/Cargo.toml @@ -31,7 +31,6 @@ tokio = { workspace = true } tracing = { workspace = true } [dev-dependencies] -anyhow = { workspace = true } futures = { workspace = true } http-body-util = { workspace = true } ic-p2p-test-utils = { path = "../test_utils" } diff --git a/rs/p2p/consensus_manager/BUILD.bazel b/rs/p2p/consensus_manager/BUILD.bazel index 84c1a9937f8f..78f96eb4277d 100644 --- a/rs/p2p/consensus_manager/BUILD.bazel +++ b/rs/p2p/consensus_manager/BUILD.bazel @@ -33,7 +33,6 @@ DEV_DEPENDENCIES = [ "//rs/p2p/test_utils", "//rs/test_utilities/logger", "//rs/types/types_test_utils", - "@crate_index//:anyhow", "@crate_index//:futures", "@crate_index//:mockall", "@crate_index//:tower", diff --git a/rs/p2p/consensus_manager/Cargo.toml b/rs/p2p/consensus_manager/Cargo.toml index ddb0d5186ae2..d3ed45295bcf 100644 --- a/rs/p2p/consensus_manager/Cargo.toml +++ b/rs/p2p/consensus_manager/Cargo.toml @@ -30,7 +30,6 @@ tokio-util = { workspace = true } tracing = { workspace = true } [dev-dependencies] -anyhow = { workspace = true } futures = { workspace = true } ic-p2p-test-utils = { path = "../test_utils" } ic-test-utilities-logger = { path = "../../test_utilities/logger" } diff --git a/rs/p2p/consensus_manager/src/sender.rs b/rs/p2p/consensus_manager/src/sender.rs index fb6b417fbe73..1172e5dc7653 100644 --- a/rs/p2p/consensus_manager/src/sender.rs +++ b/rs/p2p/consensus_manager/src/sender.rs @@ -405,12 +405,12 @@ mod available_slot_set { #[allow(clippy::disallowed_methods)] #[cfg(test)] mod tests { - use anyhow::anyhow; use axum::http::Response; use ic_interfaces::p2p::consensus::{AssembleResult, Peers}; use ic_logger::replica_logger::no_op_logger; use ic_metrics::MetricsRegistry; use ic_p2p_test_utils::{consensus::U64Artifact, mocks::MockTransport}; + use ic_quic_transport::P2PError; use ic_test_utilities_logger::with_test_replica_logger; use ic_types_test_utils::ids::{NODE_1, NODE_2}; use mockall::Sequence; @@ -561,7 +561,7 @@ mod tests { mock_transport .expect_rpc() .times(5) - .returning(move |_, _| Err(anyhow!(""))) + .returning(move |_, _| Err(P2PError::from("".to_string()))) .in_sequence(&mut seq); mock_transport.expect_rpc().times(1).returning(move |n, _| { push_tx.send(*n).unwrap(); diff --git a/rs/p2p/memory_transport/BUILD.bazel b/rs/p2p/memory_transport/BUILD.bazel index ccfa892c7d74..8f7546052f58 100644 --- a/rs/p2p/memory_transport/BUILD.bazel +++ b/rs/p2p/memory_transport/BUILD.bazel @@ -8,7 +8,6 @@ DEPENDENCIES = [ # Keep sorted. "//rs/p2p/quic_transport", "//rs/types/types", - "@crate_index//:anyhow", "@crate_index//:axum", "@crate_index//:bytes", "@crate_index//:tokio", diff --git a/rs/p2p/memory_transport/Cargo.toml b/rs/p2p/memory_transport/Cargo.toml index 0ccac887caad..b9de4257ec66 100644 --- a/rs/p2p/memory_transport/Cargo.toml +++ b/rs/p2p/memory_transport/Cargo.toml @@ -9,7 +9,6 @@ documentation.workspace = true # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -anyhow = { workspace = true } async-trait = { workspace = true } axum = { workspace = true } bytes = { workspace = true } diff --git a/rs/p2p/memory_transport/src/lib.rs b/rs/p2p/memory_transport/src/lib.rs index a5c771448f89..ff6ebe0f316f 100644 --- a/rs/p2p/memory_transport/src/lib.rs +++ b/rs/p2p/memory_transport/src/lib.rs @@ -33,7 +33,6 @@ /// ┌──────┐ │ │ ┌──────┐ /// │ Node ├───┘ └────┤ Node │ /// └──────┘ └──────┘ -use anyhow::anyhow; use async_trait::async_trait; use axum::{ body::Body, @@ -41,7 +40,7 @@ use axum::{ Router, }; use bytes::Bytes; -use ic_quic_transport::{ConnId, Transport}; +use ic_quic_transport::{ConnId, P2PError, Transport}; use ic_types::NodeId; use std::{ collections::HashMap, @@ -280,9 +279,9 @@ impl Transport for PeerTransport { &self, peer_id: &NodeId, mut request: Request, - ) -> Result, anyhow::Error> { + ) -> Result, P2PError> { if peer_id == &self.node_id { - return Err(anyhow!("Can't connect to self")); + return Err(P2PError::from("Can't connect to self".to_string())); } let (oneshot_tx, oneshot_rx) = oneshot::channel(); @@ -292,11 +291,11 @@ impl Transport for PeerTransport { .send((request, *peer_id, oneshot_tx)) .is_err() { - return Err(anyhow!("router channel closed")); + return Err(P2PError::from("router channel closed".to_string())); } match oneshot_rx.await { Ok(r) => Ok(r), - Err(_) => Err(anyhow!("channel closed")), + Err(_) => Err(P2PError::from("channel closed".to_string())), } } diff --git a/rs/p2p/quic_transport/BUILD.bazel b/rs/p2p/quic_transport/BUILD.bazel index 0a29152395d9..b3311f859a91 100644 --- a/rs/p2p/quic_transport/BUILD.bazel +++ b/rs/p2p/quic_transport/BUILD.bazel @@ -18,7 +18,6 @@ DEPENDENCIES = [ "//rs/phantom_newtype", "//rs/protobuf", "//rs/types/base_types", - "@crate_index//:anyhow", "@crate_index//:axum", "@crate_index//:bytes", "@crate_index//:futures", diff --git a/rs/p2p/quic_transport/Cargo.toml b/rs/p2p/quic_transport/Cargo.toml index 281124d0bb7b..7211513833fc 100644 --- a/rs/p2p/quic_transport/Cargo.toml +++ b/rs/p2p/quic_transport/Cargo.toml @@ -9,7 +9,6 @@ documentation.workspace = true # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -anyhow = { workspace = true } async-trait = { workspace = true } axum = { workspace = true } bytes = { workspace = true } diff --git a/rs/p2p/quic_transport/README.adoc b/rs/p2p/quic_transport/README.adoc index de0ba43ef098..220116170143 100644 --- a/rs/p2p/quic_transport/README.adoc +++ b/rs/p2p/quic_transport/README.adoc @@ -40,8 +40,8 @@ Transport is devided into two logical parts - sending and receiving. For the sending side, transport exposes the `+rpc+` API call. -Transport uses https://docs.rs/anyhow/latest/anyhow/struct.Error.html[anyhow::Error] instead of https://docs.rs/thiserror/latest/thiserror/derive.Error.html[thiserror::Error] as the error type -because the errors are transient and it is expected that the user retries on failure. +Transport uses a custom opaque error type because the errors are transient and it is expected that the user retries on failure. +We avoid using https://docs.rs/anyhow/latest/anyhow/struct.Error.html[anyhow::Error], due to it's expensive generation of backtraces. How do you make sure there are only transient errors? For example, how do you ruled out @@ -50,7 +50,7 @@ How do you make sure there are only transient errors? For example, how do you ru [source, rust] ---- - async fn rpc(&self, peer_id: &NodeId, request: Request) -> Result, anyhow::Error>; + async fn rpc(&self, peer_id: &NodeId, request: Request) -> Result, P2PError>; ---- The receiving side, is a collection of callbacks, called handlers. Each possible URI is associated with a single handler. diff --git a/rs/p2p/quic_transport/src/connection_handle.rs b/rs/p2p/quic_transport/src/connection_handle.rs index 4335988158e4..c848b9e058c6 100644 --- a/rs/p2p/quic_transport/src/connection_handle.rs +++ b/rs/p2p/quic_transport/src/connection_handle.rs @@ -13,7 +13,7 @@ use crate::{ observe_conn_error, observe_read_to_end_error, observe_stopped_error, observe_write_error, QuicTransportMetrics, INFALIBBLE, }, - ConnId, MessagePriority, ResetStreamOnDrop, MAX_MESSAGE_SIZE_BYTES, + ConnId, MessagePriority, P2PError, ResetStreamOnDrop, MAX_MESSAGE_SIZE_BYTES, }; static CONN_ID_SEQ: AtomicU64 = AtomicU64::new(1); @@ -53,7 +53,7 @@ impl ConnectionHandle { /// where connections can be managed directly by the caller. /// /// Note: The method is cancel-safe. - pub async fn rpc(&self, request: Request) -> Result, anyhow::Error> { + pub async fn rpc(&self, request: Request) -> Result, P2PError> { let _timer = self .metrics .connection_handle_duration_seconds @@ -131,7 +131,7 @@ impl ConnectionHandle { } // The function returns infallible error. -fn to_response(response_bytes: Vec) -> Result, anyhow::Error> { +fn to_response(response_bytes: Vec) -> Result, P2PError> { let response_proto = pb::HttpResponse::decode(response_bytes.as_slice())?; let status: u16 = response_proto.status_code.try_into()?; diff --git a/rs/p2p/quic_transport/src/lib.rs b/rs/p2p/quic_transport/src/lib.rs index 67a6b3e4dfe4..7383efbe0d07 100644 --- a/rs/p2p/quic_transport/src/lib.rs +++ b/rs/p2p/quic_transport/src/lib.rs @@ -30,13 +30,13 @@ //! use std::{ collections::{BTreeSet, HashMap}, - fmt::Debug, + fmt::{self, Debug, Display, Formatter}, future::Future, net::SocketAddr, + num::TryFromIntError, sync::{Arc, RwLock}, }; -use anyhow::anyhow; use async_trait::async_trait; use axum::{ http::{Request, Response}, @@ -49,7 +49,11 @@ use ic_interfaces_registry::RegistryClient; use ic_logger::{info, ReplicaLogger}; use ic_metrics::MetricsRegistry; use phantom_newtype::AmountOf; -use quinn::{AsyncUdpSocket, SendStream, VarInt}; +use quinn::{ + AsyncUdpSocket, ClosedStream, ConnectError, ConnectionError, ReadToEndError, SendStream, + StoppedError, VarInt, WriteError, +}; +use std::error::Error; use tokio::sync::watch; use tokio::task::{JoinError, JoinHandle}; use tokio_util::{sync::CancellationToken, task::task_tracker::TaskTracker}; @@ -216,13 +220,15 @@ impl Transport for QuicTransport { &self, peer_id: &NodeId, request: Request, - ) -> Result, anyhow::Error> { + ) -> Result, P2PError> { let peer = self .conn_handles .read() .unwrap() .get(peer_id) - .ok_or(anyhow!("Currently not connected to this peer"))? + .ok_or(P2PError::from( + "Currently not connected to this peer".to_string(), + ))? .clone(); peer.rpc(request).await.inspect_err(|err| { info!(every_n_seconds => 5, self.log, "Error sending rpc request to {}: {:?}", peer_id, err); @@ -239,6 +245,80 @@ impl Transport for QuicTransport { } } +#[derive(Debug)] +/// Opaque error type wrapping any P2P error, without capturing expensive backtraces. +pub struct P2PError { + inner: Box, +} + +impl P2PError { + pub fn new(error: E) -> Self + where + E: Into>, + { + P2PError { + inner: error.into(), + } + } +} + +impl Display for P2PError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.inner) + } +} + +impl Error for P2PError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + self.inner.source() + } +} + +impl From for P2PError { + fn from(value: String) -> Self { + P2PError::new(GenericError(value)) + } +} + +macro_rules! impl_from_for_p2p_error { + ($($err_ty:ty),+ $(,)?) => { + $( + impl From<$err_ty> for P2PError { + fn from(err: $err_ty) -> Self { + P2PError::new(err) + } + } + )+ + }; +} + +impl_from_for_p2p_error!( + ConnectError, + WriteError, + StoppedError, + ClosedStream, + ReadToEndError, + ConnectionError, + prost::DecodeError, + prost::UnknownEnumValue, + http::Error, + axum::Error, + TryFromIntError, +); + +#[derive(Debug, Clone)] +/// A generic error type for wrapping human-readable messages. +/// This is useful because `String` does not implement `Error`. +pub struct GenericError(pub String); + +impl Display for GenericError { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + write!(f, "{}", self.0) + } +} + +impl Error for GenericError {} + /// Low-level transport interface for exchanging messages between nodes. /// /// It intentionally uses http::Request and http::Response types. @@ -249,7 +329,7 @@ pub trait Transport: Send + Sync { &self, peer_id: &NodeId, request: Request, - ) -> Result, anyhow::Error>; + ) -> Result, P2PError>; fn peers(&self) -> Vec<(NodeId, ConnId)>; } diff --git a/rs/p2p/quic_transport/src/request_handler.rs b/rs/p2p/quic_transport/src/request_handler.rs index 61ecb2e3231a..ec3291ec322b 100644 --- a/rs/p2p/quic_transport/src/request_handler.rs +++ b/rs/p2p/quic_transport/src/request_handler.rs @@ -28,7 +28,7 @@ use crate::{ observe_conn_error, observe_read_to_end_error, observe_stopped_error, observe_write_error, QuicTransportMetrics, ERROR_TYPE_APP, INFALIBBLE, STREAM_TYPE_BIDI, }, - ConnId, ResetStreamOnDrop, MAX_MESSAGE_SIZE_BYTES, + ConnId, P2PError, ResetStreamOnDrop, MAX_MESSAGE_SIZE_BYTES, }; const QUIC_METRIC_SCRAPE_INTERVAL: Duration = Duration::from_secs(5); @@ -109,7 +109,7 @@ async fn handle_bi_stream( router: Router, mut send_stream_guard: ResetStreamOnDrop, recv_stream: RecvStream, -) -> Result<(), anyhow::Error> { +) -> Result<(), P2PError> { // Note that the 'recv_stream' is dropped before we call any method on the 'send_stream' let mut request = read_request(recv_stream, &metrics).await?; request.extensions_mut().insert::(peer_id); @@ -159,7 +159,7 @@ async fn handle_bi_stream( async fn read_request( mut recv_stream: RecvStream, metrics: &QuicTransportMetrics, -) -> Result, anyhow::Error> { +) -> Result, P2PError> { let request_bytes = recv_stream .read_to_end(MAX_MESSAGE_SIZE_BYTES) .await @@ -197,7 +197,7 @@ async fn read_request( Ok(request_builder.body(Body::from(body_bytes))?) } -async fn to_response_bytes(response: Response) -> Result, anyhow::Error> { +async fn to_response_bytes(response: Response) -> Result, P2PError> { let (parts, body) = response.into_parts(); // Check for axum error in body // TODO: Think about this. What is the error that can happen here? diff --git a/rs/p2p/test_utils/BUILD.bazel b/rs/p2p/test_utils/BUILD.bazel index 99fca89419de..ca5a4dfc7aca 100644 --- a/rs/p2p/test_utils/BUILD.bazel +++ b/rs/p2p/test_utils/BUILD.bazel @@ -29,7 +29,6 @@ DEPENDENCIES = [ "//rs/types/base_types", "//rs/types/types", "//rs/types/types_test_utils", - "@crate_index//:anyhow", "@crate_index//:axum", "@crate_index//:bytes", "@crate_index//:futures", diff --git a/rs/p2p/test_utils/Cargo.toml b/rs/p2p/test_utils/Cargo.toml index a8ac7bb41c39..08a04b79f5f3 100644 --- a/rs/p2p/test_utils/Cargo.toml +++ b/rs/p2p/test_utils/Cargo.toml @@ -7,7 +7,6 @@ description.workspace = true documentation.workspace = true [dependencies] -anyhow = { workspace = true } async-trait = { workspace = true } axum = { workspace = true } bytes = { workspace = true } diff --git a/rs/p2p/test_utils/src/mocks.rs b/rs/p2p/test_utils/src/mocks.rs index 6cc924e82438..7934fb9378ca 100644 --- a/rs/p2p/test_utils/src/mocks.rs +++ b/rs/p2p/test_utils/src/mocks.rs @@ -7,7 +7,7 @@ use ic_interfaces::p2p::{ }, state_sync::{AddChunkError, Chunk, ChunkId, Chunkable, StateSyncArtifactId, StateSyncClient}, }; -use ic_quic_transport::{ConnId, Transport}; +use ic_quic_transport::{ConnId, P2PError, Transport}; use ic_types::artifact::IdentifiableArtifact; use ic_types::NodeId; use mockall::mock; @@ -42,7 +42,7 @@ mock! { &self, peer_id: &NodeId, request: Request, - ) -> Result, anyhow::Error>; + ) -> Result, P2PError>; fn peers(&self) -> Vec<(NodeId, ConnId)>; }