From cbe6d2acef2cce968a27cecfddfde5b4bd5ea2db Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Mon, 26 Aug 2024 17:03:09 +0200 Subject: [PATCH 01/37] Add wip `QoS`-based priority-to-link dispatch impl --- DEFAULT_CONFIG.json5 | 5 + commons/zenoh-protocol/src/core/mod.rs | 61 +++- commons/zenoh-protocol/src/transport/init.rs | 14 +- io/zenoh-link-commons/src/lib.rs | 38 ++- io/zenoh-link-commons/src/unicast.rs | 13 +- io/zenoh-links/zenoh-link-quic/src/unicast.rs | 20 +- .../zenoh-link-serial/src/unicast.rs | 31 +- io/zenoh-links/zenoh-link-tcp/src/unicast.rs | 15 +- io/zenoh-links/zenoh-link-tls/src/unicast.rs | 17 +- io/zenoh-links/zenoh-link-udp/src/unicast.rs | 16 +- .../zenoh-link-unixpipe/src/unix/unicast.rs | 19 +- .../zenoh-link-unixsock_stream/src/unicast.rs | 25 +- .../zenoh-link-vsock/src/unicast.rs | 27 +- io/zenoh-links/zenoh-link-ws/src/unicast.rs | 34 ++- io/zenoh-transport/src/manager.rs | 4 +- io/zenoh-transport/src/multicast/link.rs | 14 +- io/zenoh-transport/src/multicast/mod.rs | 2 +- io/zenoh-transport/src/multicast/transport.rs | 4 +- .../src/unicast/establishment/accept.rs | 25 +- .../src/unicast/establishment/cookie.rs | 6 +- .../src/unicast/establishment/ext/qos.rs | 265 ++++++++++++------ .../src/unicast/establishment/open.rs | 20 +- io/zenoh-transport/src/unicast/link.rs | 33 +-- .../src/unicast/lowlatency/link.rs | 2 +- io/zenoh-transport/src/unicast/manager.rs | 11 +- .../src/unicast/universal/link.rs | 15 +- .../src/unicast/universal/tx.rs | 96 ++++--- 27 files changed, 545 insertions(+), 287 deletions(-) diff --git a/DEFAULT_CONFIG.json5 b/DEFAULT_CONFIG.json5 index 2ee6c5ddb5..d761f3f763 100644 --- a/DEFAULT_CONFIG.json5 +++ b/DEFAULT_CONFIG.json5 @@ -338,6 +338,11 @@ /// The supported protocols are: ["tcp" , "udp", "tls", "quic", "ws", "unixsock-stream", "vsock"] /// For example, to only enable "tls" and "quic": // protocols: ["tls", "quic"], + /// + /// Endpoints accept a "priorities" metadata value in the form of a Rust-style half-open range + /// (e.g. `2..4` signifies priorities 2 and 3). This value is used to select the link used + /// for transmission based on the priority of the message in question. + /// /// Configure the zenoh TX parameters of a link tx: { /// The resolution in bits to be used for the message sequence numbers. diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index 361da4b5da..825da6eb5c 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -24,9 +24,10 @@ use core::{ str::FromStr, }; +use serde::Serialize; pub use uhlc::{Timestamp, NTP64}; use zenoh_keyexpr::OwnedKeyExpr; -use zenoh_result::{bail, zerror}; +use zenoh_result::{bail, zerror, ZResult}; /// The unique Id of the [`HLC`](uhlc::HLC) that generated the concerned [`Timestamp`]. pub type TimestampId = uhlc::ID; @@ -308,6 +309,52 @@ pub enum Priority { Background = 7, } +#[derive(Debug, Default, Copy, Clone, Eq, Hash, PartialEq, Serialize)] +pub struct PriorityRange { + pub start: u8, + pub end: u8, +} + +impl PriorityRange { + pub fn new(start: u8, end: u8) -> ZResult { + if start >= end || start < Priority::MAX as u8 || end > Priority::MIN as u8 + 1 { + bail!("Invalid priority range: {start}..{end}") + }; + + Ok(Self { start, end }) + } + + pub fn includes(&self, priority: Priority) -> bool { + self.start <= (priority as u8) && (priority as u8) < self.end + } + + pub fn len(&self) -> usize { + (self.end - self.start) as usize + } + + pub fn is_empty(&self) -> bool { + self.end == self.start + } + + pub fn start(&self) -> u8 { + self.start + } + + pub fn end(&self) -> u8 { + self.end + } + + #[cfg(feature = "test")] + pub fn rand() -> Self { + use rand::Rng; + let mut rng = rand::thread_rng(); + let start = rng.gen_range(Priority::MAX as u8..Priority::MIN as u8); + let end = rng.gen_range((start + 1)..=Priority::MIN as u8); + + Self { start, end } + } +} + impl Priority { /// Default pub const DEFAULT: Self = Self::Data; @@ -342,7 +389,7 @@ impl TryFrom for Priority { } } -#[derive(Debug, Default, Copy, Clone, PartialEq, Eq)] +#[derive(Debug, Default, Copy, Clone, PartialEq, Eq, Hash, Serialize)] #[repr(u8)] pub enum Reliability { #[default] @@ -367,6 +414,16 @@ impl Reliability { } } +impl From for Reliability { + fn from(value: bool) -> Self { + if value { + Reliability::Reliable + } else { + Reliability::BestEffort + } + } +} + #[derive(Debug, Copy, Clone, PartialEq, Eq, Default)] pub struct Channel { pub priority: Priority, diff --git a/commons/zenoh-protocol/src/transport/init.rs b/commons/zenoh-protocol/src/transport/init.rs index 7e56bfd770..0a4e97f95e 100644 --- a/commons/zenoh-protocol/src/transport/init.rs +++ b/commons/zenoh-protocol/src/transport/init.rs @@ -126,13 +126,13 @@ pub struct InitSyn { // Extensions pub mod ext { use crate::{ - common::{ZExtUnit, ZExtZBuf}, - zextunit, zextzbuf, + common::{ZExtUnit, ZExtZ64, ZExtZBuf}, + zextunit, zextz64, zextzbuf, }; /// # QoS extension /// Used to negotiate the use of QoS - pub type QoS = zextunit!(0x1, false); + pub type QoS = zextz64!(0x1, false); /// # Shm extension /// Used as challenge for probing shared memory capabilities @@ -161,7 +161,7 @@ impl InitSyn { pub fn rand() -> Self { use rand::Rng; - use crate::common::{ZExtUnit, ZExtZBuf}; + use crate::common::{ZExtUnit, ZExtZ64, ZExtZBuf}; let mut rng = rand::thread_rng(); @@ -170,7 +170,7 @@ impl InitSyn { let zid = ZenohIdProto::default(); let resolution = Resolution::rand(); let batch_size: BatchSize = rng.gen(); - let ext_qos = rng.gen_bool(0.5).then_some(ZExtUnit::rand()); + let ext_qos = rng.gen_bool(0.5).then_some(ZExtZ64::rand()); #[cfg(feature = "shared-memory")] let ext_shm = rng.gen_bool(0.5).then_some(ZExtZBuf::rand()); let ext_auth = rng.gen_bool(0.5).then_some(ZExtZBuf::rand()); @@ -217,7 +217,7 @@ impl InitAck { pub fn rand() -> Self { use rand::Rng; - use crate::common::{ZExtUnit, ZExtZBuf}; + use crate::common::{ZExtUnit, ZExtZ64, ZExtZBuf}; let mut rng = rand::thread_rng(); @@ -231,7 +231,7 @@ impl InitAck { }; let batch_size: BatchSize = rng.gen(); let cookie = ZSlice::rand(64); - let ext_qos = rng.gen_bool(0.5).then_some(ZExtUnit::rand()); + let ext_qos = rng.gen_bool(0.5).then_some(ZExtZ64::rand()); #[cfg(feature = "shared-memory")] let ext_shm = rng.gen_bool(0.5).then_some(ZExtZBuf::rand()); let ext_auth = rng.gen_bool(0.5).then_some(ZExtZBuf::rand()); diff --git a/io/zenoh-link-commons/src/lib.rs b/io/zenoh-link-commons/src/lib.rs index 9ecf1a0dfc..c3fe1aee41 100644 --- a/io/zenoh-link-commons/src/lib.rs +++ b/io/zenoh-link-commons/src/lib.rs @@ -33,7 +33,10 @@ pub use listener::*; pub use multicast::*; use serde::Serialize; pub use unicast::*; -use zenoh_protocol::{core::Locator, transport::BatchSize}; +use zenoh_protocol::{ + core::{Locator, PriorityRange, Reliability}, + transport::BatchSize, +}; use zenoh_result::ZResult; /*************************************/ @@ -48,10 +51,11 @@ pub struct Link { pub dst: Locator, pub group: Option, pub mtu: BatchSize, - pub is_reliable: bool, pub is_streamed: bool, pub interfaces: Vec, pub auth_identifier: LinkAuthId, + pub priorities: Option, + pub reliability: Reliability, } #[async_trait] @@ -70,48 +74,40 @@ impl fmt::Display for Link { } } -impl From<&LinkUnicast> for Link { - fn from(link: &LinkUnicast) -> Link { +impl Link { + pub fn new_unicast( + link: &LinkUnicast, + priorities: Option, + reliability: Reliability, + ) -> Self { Link { src: link.get_src().to_owned(), dst: link.get_dst().to_owned(), group: None, mtu: link.get_mtu(), - is_reliable: link.is_reliable(), is_streamed: link.is_streamed(), interfaces: link.get_interface_names(), auth_identifier: link.get_auth_id().clone(), + priorities, + reliability, } } -} - -impl From for Link { - fn from(link: LinkUnicast) -> Link { - Link::from(&link) - } -} -impl From<&LinkMulticast> for Link { - fn from(link: &LinkMulticast) -> Link { + pub fn new_multicast(link: &LinkMulticast) -> Self { Link { src: link.get_src().to_owned(), dst: link.get_dst().to_owned(), group: Some(link.get_dst().to_owned()), mtu: link.get_mtu(), - is_reliable: link.is_reliable(), is_streamed: false, interfaces: vec![], auth_identifier: LinkAuthId::default(), + priorities: None, + reliability: Reliability::from(link.is_reliable()), } } } -impl From for Link { - fn from(link: LinkMulticast) -> Link { - Link::from(&link) - } -} - impl PartialEq for Link { fn eq(&self, other: &LinkUnicast) -> bool { self.src == *other.get_src() && self.dst == *other.get_dst() diff --git a/io/zenoh-link-commons/src/unicast.rs b/io/zenoh-link-commons/src/unicast.rs index 62f39bf86c..c29f4c7c14 100644 --- a/io/zenoh-link-commons/src/unicast.rs +++ b/io/zenoh-link-commons/src/unicast.rs @@ -36,7 +36,18 @@ pub trait LinkManagerUnicastTrait: Send + Sync { async fn get_listeners(&self) -> Vec; async fn get_locators(&self) -> Vec; } -pub type NewLinkChannelSender = flume::Sender; +pub type NewLinkChannelSender = flume::Sender; + +/// Notification of a new inbound connection. +/// +/// Link implementations should preserve the metadata sections of [`NewLinkUnicast::endpoint`]. +pub struct NewLinkUnicast { + /// The link created in response to a new inbound connection. + pub link: LinkUnicast, + /// Endpoint of the listener. + pub endpoint: EndPoint, +} + pub trait ConstructibleLinkManagerUnicast: Sized { fn new(new_link_sender: NewLinkChannelSender, config: T) -> ZResult; } diff --git a/io/zenoh-links/zenoh-link-quic/src/unicast.rs b/io/zenoh-links/zenoh-link-quic/src/unicast.rs index 2e0d9e0a19..3fdcfdc62d 100644 --- a/io/zenoh-links/zenoh-link-quic/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-quic/src/unicast.rs @@ -27,7 +27,7 @@ use x509_parser::prelude::*; use zenoh_core::zasynclock; use zenoh_link_commons::{ get_ip_interface_names, LinkAuthId, LinkAuthType, LinkManagerUnicastTrait, LinkUnicast, - LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, + LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, NewLinkUnicast, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -332,11 +332,14 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastQuic { // Spawn the accept loop for the listener let token = self.listeners.token.child_token(); - let c_token = token.clone(); - let c_manager = self.manager.clone(); + let task = { + let token = token.clone(); + let manager = self.manager.clone(); + let endpoint = endpoint.clone(); - let task = async move { accept_task(quic_endpoint, c_token, c_manager).await }; + async move { accept_task(endpoint, quic_endpoint, token, manager).await } + }; // Initialize the QuicAcceptor let locator = endpoint.to_locator(); @@ -364,7 +367,8 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastQuic { } async fn accept_task( - endpoint: quinn::Endpoint, + endpoint: EndPoint, + quic_endpoint: quinn::Endpoint, token: CancellationToken, manager: NewLinkChannelSender, ) -> ZResult<()> { @@ -382,7 +386,7 @@ async fn accept_task( Ok(conn) } - let src_addr = endpoint + let src_addr = quic_endpoint .local_addr() .map_err(|e| zerror!("Can not accept QUIC connections: {}", e))?; @@ -393,7 +397,7 @@ async fn accept_task( tokio::select! { _ = token.cancelled() => break, - res = accept(endpoint.accept()) => { + res = accept(quic_endpoint.accept()) => { match res { Ok(quic_conn) => { // Get the bideractional streams. Note that we don't allow unidirectional streams. @@ -429,7 +433,7 @@ async fn accept_task( )); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(LinkUnicast(link)).await { + if let Err(e) = manager.send_async(NewLinkUnicast { link: LinkUnicast(link), endpoint: endpoint.clone() }).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } diff --git a/io/zenoh-links/zenoh-link-serial/src/unicast.rs b/io/zenoh-links/zenoh-link-serial/src/unicast.rs index 5711e5fe5c..b048121a9a 100644 --- a/io/zenoh-links/zenoh-link-serial/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-serial/src/unicast.rs @@ -33,7 +33,7 @@ use z_serial::ZSerial; use zenoh_core::{zasynclock, zasyncread, zasyncwrite}; use zenoh_link_commons::{ ConstructibleLinkManagerUnicast, LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, - LinkUnicastTrait, NewLinkChannelSender, + LinkUnicastTrait, NewLinkChannelSender, NewLinkUnicast, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -332,19 +332,23 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastSerial { // Spawn the accept loop for the listener let token = CancellationToken::new(); - let c_token = token.clone(); let mut listeners = zasyncwrite!(self.listeners); - let c_path = path.clone(); - let c_manager = self.manager.clone(); - let c_listeners = self.listeners.clone(); - - let task = async move { - // Wait for the accept loop to terminate - let res = - accept_read_task(link, c_token, c_manager, c_path.clone(), is_connected).await; - zasyncwrite!(c_listeners).remove(&c_path); - res + let task = { + let token = token.clone(); + let path = path.clone(); + let manager = self.manager.clone(); + let listeners = self.listeners.clone(); + let endpoint = endpoint.clone(); + + async move { + // Wait for the accept loop to terminate + let res = + accept_read_task(endpoint, link, token, manager, path.clone(), is_connected) + .await; + zasyncwrite!(listeners).remove(&path); + res + } }; let handle = zenoh_runtime::ZRuntime::Acceptor.spawn(task); @@ -390,6 +394,7 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastSerial { } async fn accept_read_task( + endpoint: EndPoint, link: Arc, token: CancellationToken, manager: NewLinkChannelSender, @@ -423,7 +428,7 @@ async fn accept_read_task( match res { Ok(link) => { // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(LinkUnicast(link.clone())).await { + if let Err(e) = manager.send_async(NewLinkUnicast{ link: LinkUnicast(link.clone()), endpoint: endpoint.clone() }).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } diff --git a/io/zenoh-links/zenoh-link-tcp/src/unicast.rs b/io/zenoh-links/zenoh-link-tcp/src/unicast.rs index ec2e6e06b7..419c18f163 100644 --- a/io/zenoh-links/zenoh-link-tcp/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-tcp/src/unicast.rs @@ -21,7 +21,7 @@ use tokio::{ use tokio_util::sync::CancellationToken; use zenoh_link_commons::{ get_ip_interface_names, LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, LinkUnicastTrait, - ListenersUnicastIP, NewLinkChannelSender, BIND_INTERFACE, + ListenersUnicastIP, NewLinkChannelSender, NewLinkUnicast, BIND_INTERFACE, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -354,10 +354,14 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastTcp { )?; let token = self.listeners.token.child_token(); - let c_token = token.clone(); - let c_manager = self.manager.clone(); - let task = async move { accept_task(socket, c_token, c_manager).await }; + let task = { + let token = token.clone(); + let manager = self.manager.clone(); + let endpoint = endpoint.clone(); + + async move { accept_task(endpoint, socket, token, manager).await } + }; let locator = endpoint.to_locator(); self.listeners @@ -421,6 +425,7 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastTcp { } async fn accept_task( + endpoint: EndPoint, socket: TcpListener, token: CancellationToken, manager: NewLinkChannelSender, @@ -457,7 +462,7 @@ async fn accept_task( let link = Arc::new(LinkUnicastTcp::new(stream, src_addr, dst_addr)); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(LinkUnicast(link)).await { + if let Err(e) = manager.send_async(NewLinkUnicast{ link: LinkUnicast(link), endpoint: endpoint.clone() }).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } }, diff --git a/io/zenoh-links/zenoh-link-tls/src/unicast.rs b/io/zenoh-links/zenoh-link-tls/src/unicast.rs index 716eac2121..d75835e653 100644 --- a/io/zenoh-links/zenoh-link-tls/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-tls/src/unicast.rs @@ -25,7 +25,7 @@ use x509_parser::prelude::*; use zenoh_core::zasynclock; use zenoh_link_commons::{ get_ip_interface_names, LinkAuthId, LinkAuthType, LinkManagerUnicastTrait, LinkUnicast, - LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, + LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, NewLinkUnicast, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -362,12 +362,16 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastTls { let local_port = local_addr.port(); // Initialize the TlsAcceptor - let acceptor = TlsAcceptor::from(Arc::new(tls_server_config.server_config)); let token = self.listeners.token.child_token(); - let c_token = token.clone(); - let c_manager = self.manager.clone(); - let task = async move { accept_task(socket, acceptor, c_token, c_manager).await }; + let task = { + let acceptor = TlsAcceptor::from(Arc::new(tls_server_config.server_config)); + let token = token.clone(); + let manager = self.manager.clone(); + let endpoint = endpoint.clone(); + + async move { accept_task(endpoint, socket, acceptor, token, manager).await } + }; // Update the endpoint locator address let locator = Locator::new( @@ -399,6 +403,7 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastTls { } async fn accept_task( + endpoint: EndPoint, socket: TcpListener, acceptor: TlsAcceptor, token: CancellationToken, @@ -456,7 +461,7 @@ async fn accept_task( )); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(LinkUnicast(link)).await { + if let Err(e) = manager.send_async(NewLinkUnicast {link: LinkUnicast(link), endpoint: endpoint.clone()}).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } } diff --git a/io/zenoh-links/zenoh-link-udp/src/unicast.rs b/io/zenoh-links/zenoh-link-udp/src/unicast.rs index e67e821363..0c3c6a5039 100644 --- a/io/zenoh-links/zenoh-link-udp/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-udp/src/unicast.rs @@ -25,7 +25,8 @@ use tokio_util::sync::CancellationToken; use zenoh_core::{zasynclock, zlock}; use zenoh_link_commons::{ get_ip_interface_names, ConstructibleLinkManagerUnicast, LinkAuthId, LinkManagerUnicastTrait, - LinkUnicast, LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, BIND_INTERFACE, + LinkUnicast, LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, NewLinkUnicast, + BIND_INTERFACE, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -402,10 +403,14 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastUdp { )?; let token = self.listeners.token.child_token(); - let c_token = token.clone(); - let c_manager = self.manager.clone(); - let task = async move { accept_read_task(socket, c_token, c_manager).await }; + let task = { + let token = token.clone(); + let manager = self.manager.clone(); + let endpoint = endpoint.clone(); + + async move { accept_read_task(endpoint, socket, token, manager).await } + }; let locator = endpoint.to_locator(); self.listeners @@ -471,6 +476,7 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastUdp { } async fn accept_read_task( + endpoint: EndPoint, socket: UdpSocket, token: CancellationToken, manager: NewLinkChannelSender, @@ -544,7 +550,7 @@ async fn accept_read_task( LinkUnicastUdpVariant::Unconnected(unconnected), )); // Add the new link to the set of connected peers - if let Err(e) = manager.send_async(LinkUnicast(link)).await { + if let Err(e) = manager.send_async(NewLinkUnicast { link: LinkUnicast(link), endpoint: endpoint.clone() }).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } } diff --git a/io/zenoh-links/zenoh-link-unixpipe/src/unix/unicast.rs b/io/zenoh-links/zenoh-link-unixpipe/src/unix/unicast.rs index df93b9cc61..b93bb32398 100644 --- a/io/zenoh-links/zenoh-link-unixpipe/src/unix/unicast.rs +++ b/io/zenoh-links/zenoh-link-unixpipe/src/unix/unicast.rs @@ -37,7 +37,7 @@ use unix_named_pipe::{create, open_write}; use zenoh_core::{zasyncread, zasyncwrite, ResolveFuture, Wait}; use zenoh_link_commons::{ ConstructibleLinkManagerUnicast, LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, - LinkUnicastTrait, NewLinkChannelSender, + LinkUnicastTrait, NewLinkChannelSender, NewLinkUnicast, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -273,14 +273,19 @@ async fn handle_incoming_connections( endpoint.metadata(), )?; + let link = Arc::new(UnicastPipe { + r: UnsafeCell::new(dedicated_uplink), + w: UnsafeCell::new(dedicated_downlink), + local, + remote, + }); + // send newly established link to manager manager - .send_async(LinkUnicast(Arc::new(UnicastPipe { - r: UnsafeCell::new(dedicated_uplink), - w: UnsafeCell::new(dedicated_downlink), - local, - remote, - }))) + .send_async(NewLinkUnicast { + link: LinkUnicast(link), + endpoint: endpoint.clone(), + }) .await?; ZResult::Ok(()) diff --git a/io/zenoh-links/zenoh-link-unixsock_stream/src/unicast.rs b/io/zenoh-links/zenoh-link-unixsock_stream/src/unicast.rs index 5632da26f4..7bbd1f04a4 100644 --- a/io/zenoh-links/zenoh-link-unixsock_stream/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-unixsock_stream/src/unicast.rs @@ -28,6 +28,7 @@ use uuid::Uuid; use zenoh_core::{zasyncread, zasyncwrite}; use zenoh_link_commons::{ LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, LinkUnicastTrait, NewLinkChannelSender, + NewLinkUnicast, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -392,15 +393,18 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastUnixSocketStream { let c_token = token.clone(); let mut listeners = zasyncwrite!(self.listeners); - let c_manager = self.manager.clone(); - let c_listeners = self.listeners.clone(); - let c_path = local_path_str.to_owned(); - - let task = async move { - // Wait for the accept loop to terminate - let res = accept_task(socket, c_token, c_manager).await; - zasyncwrite!(c_listeners).remove(&c_path); - res + let task = { + let manager = self.manager.clone(); + let listeners = self.listeners.clone(); + let path = local_path_str.to_owned(); + let endpoint = endpoint.clone(); + + async move { + // Wait for the accept loop to terminate + let res = accept_task(endpoint, socket, c_token, manager).await; + zasyncwrite!(listeners).remove(&path); + res + } }; let handle = zenoh_runtime::ZRuntime::Acceptor.spawn(task); @@ -459,6 +463,7 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastUnixSocketStream { } async fn accept_task( + endpoint: EndPoint, socket: UnixListener, token: CancellationToken, manager: NewLinkChannelSender, @@ -515,7 +520,7 @@ async fn accept_task( )); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(LinkUnicast(link)).await { + if let Err(e) = manager.send_async(NewLinkUnicast { link: LinkUnicast(link), endpoint: endpoint.clone() }).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } diff --git a/io/zenoh-links/zenoh-link-vsock/src/unicast.rs b/io/zenoh-links/zenoh-link-vsock/src/unicast.rs index 979048b585..8ffb76d3ab 100644 --- a/io/zenoh-links/zenoh-link-vsock/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-vsock/src/unicast.rs @@ -29,6 +29,7 @@ use tokio_vsock::{ use zenoh_core::{zasyncread, zasyncwrite}; use zenoh_link_commons::{ LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, LinkUnicastTrait, NewLinkChannelSender, + NewLinkUnicast, }; use zenoh_protocol::{ core::{endpoint::Address, EndPoint, Locator}, @@ -272,20 +273,23 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastVsock { endpoint.config(), )?; let token = CancellationToken::new(); - let c_token = token.clone(); - - let c_manager = self.manager.clone(); let locator = endpoint.to_locator(); let mut listeners = zasyncwrite!(self.listeners); - let c_listeners = self.listeners.clone(); - let c_addr = addr; - let task = async move { - // Wait for the accept loop to terminate - let res = accept_task(listener, c_token, c_manager).await; - zasyncwrite!(c_listeners).remove(&c_addr); - res + + let task = { + let token = token.clone(); + let manager = self.manager.clone(); + let listeners = self.listeners.clone(); + let endpoint = endpoint.clone(); + + async move { + // Wait for the accept loop to terminate + let res = accept_task(endpoint.clone(), listener, token, manager).await; + zasyncwrite!(listeners).remove(&addr); + res + } }; let handle = zenoh_runtime::ZRuntime::Acceptor.spawn(task); @@ -329,6 +333,7 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastVsock { } async fn accept_task( + endpoint: EndPoint, mut socket: VsockListener, token: CancellationToken, manager: NewLinkChannelSender, @@ -356,7 +361,7 @@ async fn accept_task( let link = Arc::new(LinkUnicastVsock::new(stream, src_addr, dst_addr)); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(LinkUnicast(link)).await { + if let Err(e) = manager.send_async(NewLinkUnicast { link: LinkUnicast(link), endpoint: endpoint.clone() }).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } }, diff --git a/io/zenoh-links/zenoh-link-ws/src/unicast.rs b/io/zenoh-links/zenoh-link-ws/src/unicast.rs index 193c9a1724..82dc9993b3 100644 --- a/io/zenoh-links/zenoh-link-ws/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-ws/src/unicast.rs @@ -35,6 +35,7 @@ use tokio_util::sync::CancellationToken; use zenoh_core::{zasynclock, zasyncread, zasyncwrite}; use zenoh_link_commons::{ LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, LinkUnicastTrait, NewLinkChannelSender, + NewLinkUnicast, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -368,16 +369,20 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastWs { // Spawn the accept loop for the listener let token = CancellationToken::new(); - let c_token = token.clone(); - let c_manager = self.manager.clone(); - let c_listeners = self.listeners.clone(); - let c_addr = local_addr; - - let task = async move { - // Wait for the accept loop to terminate - let res = accept_task(socket, c_token, c_manager).await; - zasyncwrite!(c_listeners).remove(&c_addr); - res + + let task = { + let token = token.clone(); + let manager = self.manager.clone(); + let listeners = self.listeners.clone(); + let addr = local_addr; + let endpoint = endpoint.clone(); + + async move { + // Wait for the accept loop to terminate + let res = accept_task(endpoint, socket, token, manager).await; + zasyncwrite!(listeners).remove(&addr); + res + } }; let handle = zenoh_runtime::ZRuntime::Acceptor.spawn(task); @@ -467,6 +472,7 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastWs { } async fn accept_task( + endpoint: EndPoint, socket: TcpListener, token: CancellationToken, manager: NewLinkChannelSender, @@ -535,7 +541,13 @@ async fn accept_task( let link = Arc::new(LinkUnicastWs::new(stream, src_addr, dst_addr)); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(LinkUnicast(link)).await { + if let Err(e) = manager + .send_async(NewLinkUnicast { + link: LinkUnicast(link), + endpoint: endpoint.clone(), + }) + .await + { tracing::error!("{}-{}: {}", file!(), line!(), e) } } diff --git a/io/zenoh-transport/src/manager.rs b/io/zenoh-transport/src/manager.rs index 305ccab574..3746092d4a 100644 --- a/io/zenoh-transport/src/manager.rs +++ b/io/zenoh-transport/src/manager.rs @@ -419,8 +419,8 @@ impl TransportManager { loop { tokio::select! { res = new_unicast_link_receiver.recv_async() => { - if let Ok(link) = res { - this.handle_new_link_unicast(link).await; + if let Ok(new_link) = res { + this.handle_new_link_unicast(new_link).await; } } _ = cancellation_token.cancelled() => { break; } diff --git a/io/zenoh-transport/src/multicast/link.rs b/io/zenoh-transport/src/multicast/link.rs index d0d5ef4fb0..f2258b6935 100644 --- a/io/zenoh-transport/src/multicast/link.rs +++ b/io/zenoh-transport/src/multicast/link.rs @@ -21,7 +21,7 @@ use std::{ use tokio::task::JoinHandle; use zenoh_buffers::{BBuf, ZSlice, ZSliceBuffer}; use zenoh_core::{zcondfeat, zlock}; -use zenoh_link::{Link, LinkMulticast, Locator}; +use zenoh_link::{LinkMulticast, Locator}; use zenoh_protocol::{ core::{Bits, Priority, Resolution, WhatAmI, ZenohIdProto}, transport::{BatchSize, Close, Join, PrioritySn, TransportMessage, TransportSn}, @@ -126,18 +126,6 @@ impl fmt::Debug for TransportLinkMulticast { } } -impl From<&TransportLinkMulticast> for Link { - fn from(link: &TransportLinkMulticast) -> Self { - Link::from(&link.link) - } -} - -impl From for Link { - fn from(link: TransportLinkMulticast) -> Self { - Link::from(link.link) - } -} - pub(crate) struct TransportLinkMulticastTx { pub(crate) inner: TransportLinkMulticast, pub(crate) buffer: Option, diff --git a/io/zenoh-transport/src/multicast/mod.rs b/io/zenoh-transport/src/multicast/mod.rs index 78d76bb6c8..fbb656264d 100644 --- a/io/zenoh-transport/src/multicast/mod.rs +++ b/io/zenoh-transport/src/multicast/mod.rs @@ -92,7 +92,7 @@ impl TransportMulticast { #[inline(always)] pub fn get_link(&self) -> ZResult { let transport = self.get_transport()?; - Ok(transport.get_link().into()) + Ok(Link::new_multicast(&transport.get_link().link)) } #[inline(always)] diff --git a/io/zenoh-transport/src/multicast/transport.rs b/io/zenoh-transport/src/multicast/transport.rs index f0dfec4813..36b9dbbea0 100644 --- a/io/zenoh-transport/src/multicast/transport.rs +++ b/io/zenoh-transport/src/multicast/transport.rs @@ -333,7 +333,7 @@ impl TransportMulticastInner { /* PEER */ /*************************************/ pub(super) fn new_peer(&self, locator: &Locator, join: Join) -> ZResult<()> { - let mut link = Link::from(self.get_link()); + let mut link = Link::new_multicast(&self.get_link().link); link.dst = locator.clone(); let is_shm = zcondfeat!("shared-memory", join.ext_shm.is_some(), false); @@ -452,7 +452,7 @@ impl TransportMulticastInner { zread!(self.peers) .values() .map(|p| { - let mut link = Link::from(self.get_link()); + let mut link = Link::new_multicast(&self.get_link().link); link.dst = p.locator.clone(); TransportPeer { diff --git a/io/zenoh-transport/src/unicast/establishment/accept.rs b/io/zenoh-transport/src/unicast/establishment/accept.rs index 9d34896d95..711fb000e4 100644 --- a/io/zenoh-transport/src/unicast/establishment/accept.rs +++ b/io/zenoh-transport/src/unicast/establishment/accept.rs @@ -20,9 +20,9 @@ use zenoh_buffers::{reader::HasReader, writer::HasWriter, ZSlice}; use zenoh_codec::{RCodec, WCodec, Zenoh080}; use zenoh_core::{zasynclock, zcondfeat, zerror}; use zenoh_crypto::{BlockCipher, PseudoRng}; -use zenoh_link::LinkUnicast; +use zenoh_link::{EndPoint, LinkUnicast}; use zenoh_protocol::{ - core::{Field, Resolution, WhatAmI, ZenohIdProto}, + core::{Field, Reliability, Resolution, WhatAmI, ZenohIdProto}, transport::{ batch_size, close::{self, Close}, @@ -55,7 +55,7 @@ pub(super) type AcceptError = (zenoh_result::Error, Option); struct StateTransport { batch_size: BatchSize, resolution: Resolution, - ext_qos: ext::qos::StateAccept, + ext_qos: ext::qos::QoS, #[cfg(feature = "transport_multilink")] ext_mlink: ext::multilink::StateAccept, #[cfg(feature = "shared-memory")] @@ -641,17 +641,24 @@ impl<'a, 'b: 'a> AcceptFsm for &'a mut AcceptLink<'b> { } } -pub(crate) async fn accept_link(link: LinkUnicast, manager: &TransportManager) -> ZResult<()> { +pub(crate) async fn accept_link( + endpoint: EndPoint, + link: LinkUnicast, + manager: &TransportManager, +) -> ZResult<()> { + let direction = TransportLinkUnicastDirection::Inbound; let mtu = link.get_mtu(); let is_streamed = link.is_streamed(); let config = TransportLinkUnicastConfig { - direction: TransportLinkUnicastDirection::Inbound, + direction, batch: BatchConfig { mtu, is_streamed, #[cfg(feature = "transport_compression")] is_compression: false, }, + priorities: None, + reliability: Reliability::from(link.is_reliable()), }; let mut link = TransportLinkUnicast::new(link, config); let mut fsm = AcceptLink { @@ -699,7 +706,7 @@ pub(crate) async fn accept_link(link: LinkUnicast, manager: &TransportManager) - transport: StateTransport { batch_size, resolution: manager.config.resolution, - ext_qos: ext::qos::StateAccept::new(manager.config.unicast.is_qos), + ext_qos: ext::qos::QoS::new(manager.config.unicast.is_qos, &endpoint)?, #[cfg(feature = "transport_multilink")] ext_mlink: manager .state @@ -767,7 +774,7 @@ pub(crate) async fn accept_link(link: LinkUnicast, manager: &TransportManager) - whatami: osyn_out.other_whatami, sn_resolution: state.transport.resolution.get(Field::FrameSN), tx_initial_sn: oack_out.open_ack.initial_sn, - is_qos: state.transport.ext_qos.is_qos(), + is_qos: state.transport.ext_qos.is_enabled(), #[cfg(feature = "transport_multilink")] multilink: state.transport.ext_mlink.multilink(), #[cfg(feature = "shared-memory")] @@ -781,13 +788,15 @@ pub(crate) async fn accept_link(link: LinkUnicast, manager: &TransportManager) - }; let a_config = TransportLinkUnicastConfig { - direction: TransportLinkUnicastDirection::Inbound, + direction, batch: BatchConfig { mtu: state.transport.batch_size, is_streamed, #[cfg(feature = "transport_compression")] is_compression: state.link.ext_compression.is_compression(), }, + priorities: state.transport.ext_qos.priorities(), + reliability: Reliability::from(link.link.is_reliable()), }; let a_link = link.reconfigure(a_config); let s_link = format!("{:?}", a_link); diff --git a/io/zenoh-transport/src/unicast/establishment/cookie.rs b/io/zenoh-transport/src/unicast/establishment/cookie.rs index 4220f8e08b..7fb84f258e 100644 --- a/io/zenoh-transport/src/unicast/establishment/cookie.rs +++ b/io/zenoh-transport/src/unicast/establishment/cookie.rs @@ -34,7 +34,7 @@ pub(crate) struct Cookie { pub(crate) batch_size: BatchSize, pub(crate) nonce: u64, // Extensions - pub(crate) ext_qos: ext::qos::StateAccept, + pub(crate) ext_qos: ext::qos::QoS, #[cfg(feature = "transport_multilink")] pub(crate) ext_mlink: ext::multilink::StateAccept, #[cfg(feature = "shared-memory")] @@ -90,7 +90,7 @@ where let batch_size: BatchSize = self.read(&mut *reader)?; let nonce: u64 = self.read(&mut *reader)?; // Extensions - let ext_qos: ext::qos::StateAccept = self.read(&mut *reader)?; + let ext_qos: ext::qos::QoS = self.read(&mut *reader)?; #[cfg(feature = "transport_multilink")] let ext_mlink: ext::multilink::StateAccept = self.read(&mut *reader)?; #[cfg(feature = "shared-memory")] @@ -178,7 +178,7 @@ impl Cookie { resolution: Resolution::rand(), batch_size: rng.gen(), nonce: rng.gen(), - ext_qos: ext::qos::StateAccept::rand(), + ext_qos: ext::qos::QoS::rand(), #[cfg(feature = "transport_multilink")] ext_mlink: ext::multilink::StateAccept::rand(), #[cfg(feature = "shared-memory")] diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index f749073805..5bebe00bb0 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -14,13 +14,19 @@ use core::marker::PhantomData; use async_trait::async_trait; +use serde::Serialize; use zenoh_buffers::{ reader::{DidntRead, Reader}, writer::{DidntWrite, Writer}, }; use zenoh_codec::{RCodec, WCodec, Zenoh080}; -use zenoh_protocol::transport::{init, open}; -use zenoh_result::Error as ZError; +use zenoh_core::{bail, zerror}; +use zenoh_link::EndPoint; +use zenoh_protocol::{ + core::PriorityRange, + transport::{init, open}, +}; +use zenoh_result::{Error as ZError, ZResult}; use crate::unicast::establishment::{AcceptFsm, OpenFsm}; @@ -35,21 +41,148 @@ impl<'a> QoSFsm<'a> { } } -/*************************************/ -/* OPEN */ -/*************************************/ -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(crate) struct StateOpen { - is_qos: bool, +#[derive(Clone, Debug, PartialEq, Eq, Serialize)] +pub(crate) enum QoS { + Disabled, + Enabled { priorities: Option }, +} + +impl QoS { + pub(crate) fn new(is_qos: bool, endpoint: &EndPoint) -> ZResult { + if !is_qos { + Ok(Self::Disabled) + } else { + const PRIORITY_METADATA_KEY: &str = "priorities"; + + let endpoint_metadata = endpoint.metadata(); + + let Some(mut priorities) = endpoint_metadata + .get(PRIORITY_METADATA_KEY) + .map(|metadata| metadata.split("..")) + else { + return Ok(Self::Enabled { priorities: None }); + }; + let start = priorities + .next() + .ok_or(zerror!("Invalid priority range syntax"))? + .parse::()?; + + let end = priorities + .next() + .ok_or(zerror!("Invalid priority range syntax"))? + .parse::()?; + + if priorities.next().is_some() { + bail!("Invalid priority range syntax") + }; + + Ok(Self::Enabled { + priorities: Some(PriorityRange::new(start, end)?), + }) + } + } + + fn try_from_u64(value: u64) -> ZResult { + match value { + 0b00_u64 => Ok(QoS::Disabled), + 0b01_u64 => Ok(QoS::Enabled { priorities: None }), + mut value if value & 0b10_u64 != 0 => { + value >>= 2; + let start = (value & 0xff) as u8; + let end = ((value & 0xff00) >> 8) as u8; + + Ok(QoS::Enabled { + priorities: Some(PriorityRange::new(start, end)?), + }) + } + _ => Err(zerror!("invalid QoS").into()), + } + } + + /// Encodes [`QoS`] as a [`u64`]. + /// + /// The two least significant bits are used to discrimnate three states: + /// + /// 1. QoS is disabled + /// 2. QoS is enabled but no priority range is available + /// 3. QoS is enabled and priority information is range, in which case the next 16 least + /// significant bits are used to encode the priority range. + fn to_u64(&self) -> u64 { + match self { + QoS::Disabled => 0b00_u64, + QoS::Enabled { priorities: None } => 0b01_u64, + QoS::Enabled { + priorities: Some(range), + } => ((range.end() as u64) << 10) | ((range.start() as u64) << 2) | 0b10_u64, + } + } + + fn unify(&self, other: &Self) -> Self { + match (self, other) { + (QoS::Disabled, QoS::Disabled) => QoS::Disabled, + (QoS::Disabled, QoS::Enabled { priorities }) + | (QoS::Enabled { priorities }, QoS::Disabled) => QoS::Enabled { + priorities: *priorities, + }, + (QoS::Enabled { priorities: lhs }, QoS::Enabled { priorities: rhs }) => { + if lhs == rhs { + QoS::Enabled { priorities: *rhs } + } else { + QoS::Enabled { priorities: None } + } + } + } + } + + pub(crate) fn is_enabled(&self) -> bool { + matches!(self, QoS::Enabled { .. }) + } + + pub(crate) fn priorities(&self) -> Option { + match self { + QoS::Disabled | QoS::Enabled { priorities: None } => None, + QoS::Enabled { + priorities: Some(priorities), + } => Some(*priorities), + } + } + + #[cfg(test)] + pub(crate) fn rand() -> Self { + use rand::Rng; + let mut rng = rand::thread_rng(); + if rng.gen_bool(0.5) { + QoS::Disabled + } else if rng.gen_bool(0.5) { + QoS::Enabled { priorities: None } + } else { + QoS::Enabled { + priorities: Some(PriorityRange::rand()), + } + } + } } -impl StateOpen { - pub(crate) const fn new(is_qos: bool) -> Self { - Self { is_qos } +// Codec +impl WCodec<&QoS, &mut W> for Zenoh080 +where + W: Writer, +{ + type Output = Result<(), DidntWrite>; + + fn write(self, writer: &mut W, x: &QoS) -> Self::Output { + self.write(writer, &x.to_u64()) } +} + +impl RCodec for Zenoh080 +where + R: Reader, +{ + type Error = DidntRead; - pub(crate) const fn is_qos(&self) -> bool { - self.is_qos + fn read(self, reader: &mut R) -> Result { + QoS::try_from_u64(self.read(reader)?).map_err(|_| DidntRead) } } @@ -57,28 +190,39 @@ impl StateOpen { impl<'a> OpenFsm for &'a QoSFsm<'a> { type Error = ZError; - type SendInitSynIn = &'a StateOpen; + type SendInitSynIn = &'a QoS; type SendInitSynOut = Option; async fn send_init_syn( self, state: Self::SendInitSynIn, ) -> Result { - let output = state.is_qos.then_some(init::ext::QoS::new()); - Ok(output) + if state.is_enabled() { + Ok(Some(init::ext::QoS::new(state.to_u64()))) + } else { + Ok(None) + } } - type RecvInitAckIn = (&'a mut StateOpen, Option); + type RecvInitAckIn = (&'a mut QoS, Option); type RecvInitAckOut = (); async fn recv_init_ack( self, input: Self::RecvInitAckIn, ) -> Result { - let (state, other_ext) = input; - state.is_qos &= other_ext.is_some(); + let (state_self, other_ext) = input; + + let state_other = if let Some(other_ext) = other_ext { + QoS::try_from_u64(other_ext.value)? + } else { + QoS::Disabled + }; + + *state_self = state_self.unify(&state_other); + Ok(()) } - type SendOpenSynIn = &'a StateOpen; + type SendOpenSynIn = &'a QoS; type SendOpenSynOut = Option; async fn send_open_syn( self, @@ -87,7 +231,7 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { Ok(None) } - type RecvOpenAckIn = (&'a mut StateOpen, Option); + type RecvOpenAckIn = (&'a mut QoS, Option); type RecvOpenAckOut = (); async fn recv_open_ack( self, @@ -97,84 +241,43 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { } } -/*************************************/ -/* ACCEPT */ -/*************************************/ -#[derive(Clone, Copy, Debug, PartialEq, Eq)] -pub(crate) struct StateAccept { - is_qos: bool, -} - -impl StateAccept { - pub(crate) const fn new(is_qos: bool) -> Self { - Self { is_qos } - } - - pub(crate) const fn is_qos(&self) -> bool { - self.is_qos - } - - #[cfg(test)] - pub(crate) fn rand() -> Self { - use rand::Rng; - let mut rng = rand::thread_rng(); - Self::new(rng.gen_bool(0.5)) - } -} - -// Codec -impl WCodec<&StateAccept, &mut W> for Zenoh080 -where - W: Writer, -{ - type Output = Result<(), DidntWrite>; - - fn write(self, writer: &mut W, x: &StateAccept) -> Self::Output { - let is_qos = u8::from(x.is_qos); - self.write(&mut *writer, is_qos)?; - Ok(()) - } -} - -impl RCodec for Zenoh080 -where - R: Reader, -{ - type Error = DidntRead; - - fn read(self, reader: &mut R) -> Result { - let is_qos: u8 = self.read(&mut *reader)?; - let is_qos = is_qos == 1; - Ok(StateAccept { is_qos }) - } -} - #[async_trait] impl<'a> AcceptFsm for &'a QoSFsm<'a> { type Error = ZError; - type RecvInitSynIn = (&'a mut StateAccept, Option); + type RecvInitSynIn = (&'a mut QoS, Option); type RecvInitSynOut = (); async fn recv_init_syn( self, input: Self::RecvInitSynIn, ) -> Result { - let (state, other_ext) = input; - state.is_qos &= other_ext.is_some(); + let (state_self, other_ext) = input; + + let state_other = if let Some(other_ext) = other_ext { + QoS::try_from_u64(other_ext.value)? + } else { + QoS::Disabled + }; + + *state_self = state_self.unify(&state_other); + Ok(()) } - type SendInitAckIn = &'a StateAccept; + type SendInitAckIn = &'a QoS; type SendInitAckOut = Option; async fn send_init_ack( self, state: Self::SendInitAckIn, ) -> Result { - let output = state.is_qos.then_some(init::ext::QoS::new()); - Ok(output) + if state.is_enabled() { + Ok(Some(init::ext::QoS::new(state.to_u64()))) + } else { + Ok(None) + } } - type RecvOpenSynIn = (&'a mut StateAccept, Option); + type RecvOpenSynIn = (&'a mut QoS, Option); type RecvOpenSynOut = (); async fn recv_open_syn( self, @@ -183,7 +286,7 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { Ok(()) } - type SendOpenAckIn = &'a StateAccept; + type SendOpenAckIn = &'a QoS; type SendOpenAckOut = Option; async fn send_open_ack( self, diff --git a/io/zenoh-transport/src/unicast/establishment/open.rs b/io/zenoh-transport/src/unicast/establishment/open.rs index ff73e213c2..7d885c3139 100644 --- a/io/zenoh-transport/src/unicast/establishment/open.rs +++ b/io/zenoh-transport/src/unicast/establishment/open.rs @@ -18,9 +18,9 @@ use zenoh_buffers::ZSlice; #[cfg(feature = "transport_auth")] use zenoh_core::zasynclock; use zenoh_core::{zcondfeat, zerror}; -use zenoh_link::LinkUnicast; +use zenoh_link::{EndPoint, LinkUnicast}; use zenoh_protocol::{ - core::{Field, Resolution, WhatAmI, ZenohIdProto}, + core::{Field, Reliability, Resolution, WhatAmI, ZenohIdProto}, transport::{ batch_size, close, BatchSize, Close, InitSyn, OpenSyn, TransportBody, TransportMessage, TransportSn, @@ -52,7 +52,7 @@ type OpenError = (zenoh_result::Error, Option); struct StateTransport { batch_size: BatchSize, resolution: Resolution, - ext_qos: ext::qos::StateOpen, + ext_qos: ext::qos::QoS, #[cfg(feature = "transport_multilink")] ext_mlink: ext::multilink::StateOpen, #[cfg(feature = "shared-memory")] @@ -537,18 +537,22 @@ impl<'a, 'b: 'a> OpenFsm for &'a mut OpenLink<'b> { } pub(crate) async fn open_link( + endpoint: EndPoint, link: LinkUnicast, manager: &TransportManager, ) -> ZResult { + let direction = TransportLinkUnicastDirection::Outbound; let is_streamed = link.is_streamed(); let config = TransportLinkUnicastConfig { - direction: TransportLinkUnicastDirection::Outbound, + direction, batch: BatchConfig { mtu: link.get_mtu(), is_streamed, #[cfg(feature = "transport_compression")] is_compression: false, // Perform the exchange Init/Open exchange with no compression }, + priorities: None, + reliability: Reliability::from(link.is_reliable()), }; let mut link = TransportLinkUnicast::new(link, config); let mut fsm = OpenLink { @@ -582,7 +586,7 @@ pub(crate) async fn open_link( transport: StateTransport { batch_size, resolution: manager.config.resolution, - ext_qos: ext::qos::StateOpen::new(manager.config.unicast.is_qos), + ext_qos: ext::qos::QoS::new(manager.config.unicast.is_qos, &endpoint)?, #[cfg(feature = "transport_multilink")] ext_mlink: manager .state @@ -650,7 +654,7 @@ pub(crate) async fn open_link( whatami: iack_out.other_whatami, sn_resolution: state.transport.resolution.get(Field::FrameSN), tx_initial_sn: osyn_out.mine_initial_sn, - is_qos: state.transport.ext_qos.is_qos(), + is_qos: state.transport.ext_qos.is_enabled(), #[cfg(feature = "transport_multilink")] multilink: state.transport.ext_mlink.multilink(), #[cfg(feature = "shared-memory")] @@ -664,13 +668,15 @@ pub(crate) async fn open_link( }; let o_config = TransportLinkUnicastConfig { - direction: TransportLinkUnicastDirection::Outbound, + direction, batch: BatchConfig { mtu: state.transport.batch_size, is_streamed, #[cfg(feature = "transport_compression")] is_compression: state.link.ext_compression.is_compression(), }, + priorities: state.transport.ext_qos.priorities(), + reliability: Reliability::from(link.link.is_reliable()), }; let o_link = link.reconfigure(o_config); let s_link = format!("{:?}", o_link); diff --git a/io/zenoh-transport/src/unicast/link.rs b/io/zenoh-transport/src/unicast/link.rs index 736360db63..6e8425f056 100644 --- a/io/zenoh-transport/src/unicast/link.rs +++ b/io/zenoh-transport/src/unicast/link.rs @@ -16,7 +16,10 @@ use std::{fmt, sync::Arc}; use zenoh_buffers::{BBuf, ZSlice, ZSliceBuffer}; use zenoh_core::zcondfeat; use zenoh_link::{Link, LinkUnicast}; -use zenoh_protocol::transport::{BatchSize, Close, OpenAck, TransportMessage}; +use zenoh_protocol::{ + core::{PriorityRange, Reliability}, + transport::{BatchSize, Close, OpenAck, TransportMessage}, +}; use zenoh_result::{zerror, ZResult}; use crate::common::batch::{BatchConfig, Decode, Encode, Finalize, RBatch, WBatch}; @@ -32,6 +35,8 @@ pub(crate) struct TransportLinkUnicastConfig { // Inbound / outbound pub(crate) direction: TransportLinkUnicastDirection, pub(crate) batch: BatchConfig, + pub(crate) priorities: Option, + pub(crate) reliability: Reliability, } #[derive(Clone, PartialEq, Eq)] @@ -55,7 +60,7 @@ impl TransportLinkUnicast { } pub(crate) fn link(&self) -> Link { - (&self.link).into() + Link::new_unicast(&self.link, self.config.priorities, self.config.reliability) } pub(crate) fn tx(&self) -> TransportLinkUnicastTx { @@ -77,7 +82,7 @@ impl TransportLinkUnicast { pub(crate) fn rx(&self) -> TransportLinkUnicastRx { TransportLinkUnicastRx { link: self.link.clone(), - batch: self.config.batch, + config: self.config.clone(), } } @@ -121,18 +126,6 @@ impl fmt::Debug for TransportLinkUnicast { } } -impl From<&TransportLinkUnicast> for Link { - fn from(link: &TransportLinkUnicast) -> Self { - Link::from(&link.link) - } -} - -impl From for Link { - fn from(link: TransportLinkUnicast) -> Self { - Link::from(&link.link) - } -} - impl PartialEq for TransportLinkUnicast { fn eq(&self, other: &Link) -> bool { &other.src == self.link.get_src() && &other.dst == self.link.get_dst() @@ -201,7 +194,7 @@ impl fmt::Debug for TransportLinkUnicastTx { pub(crate) struct TransportLinkUnicastRx { pub(crate) link: LinkUnicast, - pub(crate) batch: BatchConfig, + pub(crate) config: TransportLinkUnicastConfig, } impl TransportLinkUnicastRx { @@ -235,7 +228,7 @@ impl TransportLinkUnicastRx { let buffer = ZSlice::new(Arc::new(into), 0, end) .map_err(|_| zerror!("{ERR}{self}. ZSlice index(es) out of bounds"))?; - let mut batch = RBatch::new(self.batch, buffer); + let mut batch = RBatch::new(self.config.batch, buffer); batch .initialize(buff) .map_err(|e| zerror!("{ERR}{self}. {e}."))?; @@ -246,7 +239,7 @@ impl TransportLinkUnicastRx { } pub async fn recv(&mut self) -> ZResult { - let mtu = self.batch.mtu as usize; + let mtu = self.config.batch.mtu as usize; let mut batch = self .recv_batch(|| zenoh_buffers::vec::uninit(mtu).into_boxed_slice()) .await?; @@ -259,7 +252,7 @@ impl TransportLinkUnicastRx { impl fmt::Display for TransportLinkUnicastRx { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - write!(f, "{}:{:?}", self.link, self.batch) + write!(f, "{}:{:?}", self.link, self.config) } } @@ -267,7 +260,7 @@ impl fmt::Debug for TransportLinkUnicastRx { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { f.debug_struct("TransportLinkUnicastRx") .field("link", &self.link) - .field("config", &self.batch) + .field("config", &self.config) .finish() } } diff --git a/io/zenoh-transport/src/unicast/lowlatency/link.rs b/io/zenoh-transport/src/unicast/lowlatency/link.rs index 3ba1cd724f..950d11f3c2 100644 --- a/io/zenoh-transport/src/unicast/lowlatency/link.rs +++ b/io/zenoh-transport/src/unicast/lowlatency/link.rs @@ -152,7 +152,7 @@ impl TransportUnicastLowlatency { // The pool of buffers let pool = { - let mtu = link_rx.batch.mtu as usize; + let mtu = link_rx.config.batch.mtu as usize; let mut n = rx_buffer_size / mtu; if rx_buffer_size % mtu != 0 { n += 1; diff --git a/io/zenoh-transport/src/unicast/manager.rs b/io/zenoh-transport/src/unicast/manager.rs index bff221323e..d0fbecad5f 100644 --- a/io/zenoh-transport/src/unicast/manager.rs +++ b/io/zenoh-transport/src/unicast/manager.rs @@ -615,7 +615,7 @@ impl TransportManager { }, { tracing::debug!( - "New transport opened between {} and {} - whatami: {}, sn resolution: {:?}, initial sn: {:?}, qos: {}, multilink: {}, lowlatency: {}", + "New transport opened between {} and {} - whatami: {}, sn resolution: {:?}, initial sn: {:?}, qos: {:?}, multilink: {}, lowlatency: {}", self.config.zid, config.zid, config.whatami, @@ -707,9 +707,9 @@ impl TransportManager { }; // Create a new link associated by calling the Link Manager - let link = manager.new_link(endpoint).await?; + let link = manager.new_link(endpoint.clone()).await?; // Open the link - super::establishment::open::open_link(link, self).await + super::establishment::open::open_link(endpoint, link, self).await } pub async fn get_transport_unicast(&self, peer: &ZenohIdProto) -> Option { @@ -736,7 +736,8 @@ impl TransportManager { Ok(()) } - pub(crate) async fn handle_new_link_unicast(&self, link: LinkUnicast) { + pub(crate) async fn handle_new_link_unicast(&self, new_link: NewLinkUnicast) { + let NewLinkUnicast { link, endpoint } = new_link; let incoming_counter = self.state.unicast.incoming.clone(); if incoming_counter.load(SeqCst) >= self.config.unicast.accept_pending { // We reached the limit of concurrent incoming transport, this means two things: @@ -759,7 +760,7 @@ impl TransportManager { .spawn_with_rt(zenoh_runtime::ZRuntime::Acceptor, async move { if tokio::time::timeout( c_manager.config.unicast.accept_timeout, - super::establishment::accept::accept_link(link, &c_manager), + super::establishment::accept::accept_link(endpoint, link, &c_manager), ) .await .is_err() diff --git a/io/zenoh-transport/src/unicast/universal/link.rs b/io/zenoh-transport/src/unicast/universal/link.rs index fff842c255..960b2e1f34 100644 --- a/io/zenoh-transport/src/unicast/universal/link.rs +++ b/io/zenoh-transport/src/unicast/universal/link.rs @@ -15,6 +15,7 @@ use std::time::Duration; use tokio_util::{sync::CancellationToken, task::TaskTracker}; use zenoh_buffers::ZSliceBuffer; +use zenoh_link::Link; use zenoh_protocol::transport::{KeepAlive, TransportMessage}; use zenoh_result::{zerror, ZResult}; use zenoh_sync::{RecyclingObject, RecyclingObjectPool}; @@ -113,6 +114,8 @@ impl TransportLinkUnicastUniversal { } pub(super) fn start_rx(&mut self, transport: TransportUnicastUniversal, lease: Duration) { + let priorities = self.link.config.priorities; + let reliability = self.link.config.reliability; let mut rx = self.link.rx(); let token = self.token.clone(); let task = async move { @@ -133,8 +136,12 @@ impl TransportLinkUnicastUniversal { // Spawn a task to avoid a deadlock waiting for this same task // to finish in the close() joining its handle // WARN: Must be spawned on RX - zenoh_runtime::ZRuntime::RX - .spawn(async move { transport.del_link((&rx.link).into()).await }); + + zenoh_runtime::ZRuntime::RX.spawn(async move { + transport + .del_link(Link::new_unicast(&rx.link, priorities, reliability)) + .await + }); // // WARN: This ZRuntime blocks // zenoh_runtime::ZRuntime::Net @@ -248,14 +255,14 @@ async fn rx_task( } // The pool of buffers - let mtu = link.batch.mtu as usize; + let mtu = link.config.batch.mtu as usize; let mut n = rx_buffer_size / mtu; if rx_buffer_size % mtu != 0 { n += 1; } let pool = RecyclingObjectPool::new(n, || vec![0_u8; mtu].into_boxed_slice()); - let l = (&link.link).into(); + let l = Link::new_unicast(&link.link, link.config.priorities, link.config.reliability); loop { tokio::select! { diff --git a/io/zenoh-transport/src/unicast/universal/tx.rs b/io/zenoh-transport/src/unicast/universal/tx.rs index f7754489ef..4fadcdb147 100644 --- a/io/zenoh-transport/src/unicast/universal/tx.rs +++ b/io/zenoh-transport/src/unicast/universal/tx.rs @@ -11,51 +11,81 @@ // Contributors: // ZettaScale Zenoh Team, // -use zenoh_core::zread; -use zenoh_protocol::network::NetworkMessage; +use zenoh_protocol::{ + core::{PriorityRange, Reliability}, + network::NetworkMessage, +}; use super::transport::TransportUnicastUniversal; #[cfg(feature = "shared-memory")] use crate::shm::map_zmsg_to_partner; +use crate::unicast::transport_unicast_inner::TransportUnicastTrait; impl TransportUnicastUniversal { fn schedule_on_link(&self, msg: NetworkMessage) -> bool { - macro_rules! zpush { - ($guard:expr, $pipeline:expr, $msg:expr) => { - // Drop the guard before the push_zenoh_message since - // the link could be congested and this operation could - // block for fairly long time - let pl = $pipeline.clone(); - drop($guard); - tracing::trace!("Scheduled: {:?}", $msg); - return pl.push_network_message($msg); - }; - } + let transport_links = self + .links + .read() + .expect("reading `TransportUnicastUniversal::links` should not fail"); - let guard = zread!(self.links); - // First try to find the best match between msg and link reliability - if let Some(pl) = guard.iter().find_map(|tl| { - if msg.is_reliable() == tl.link.link.is_reliable() { - Some(&tl.pipeline) - } else { - None - } - }) { - zpush!(guard, pl, msg); - } + let msg_reliability = Reliability::from(msg.is_reliable()); - // No best match found, take the first available link - if let Some(pl) = guard.iter().map(|tl| &tl.pipeline).next() { - zpush!(guard, pl, msg); - } + let (full_match, partial_match, any_match) = transport_links.iter().enumerate().fold( + (None::<(_, PriorityRange)>, None, None), + |(mut full_match, mut partial_match, mut any_match), (i, transport_link)| { + let reliability = transport_link.link.config.reliability == msg_reliability; + let priorities = transport_link + .link + .config + .priorities + .and_then(|range| range.includes(msg.priority()).then_some(range)); - // No Link found - tracing::trace!( - "Message dropped because the transport has no links: {}", - msg + match (reliability, priorities) { + (true, Some(priorities)) => { + match full_match { + Some((_, r)) if priorities.len() < r.len() => { + full_match = Some((i, priorities)) + } + None => full_match = Some((i, priorities)), + _ => {} + }; + } + (true, None) if partial_match.is_none() => partial_match = Some(i), + _ if any_match.is_none() => any_match = Some(i), + _ => {} + }; + + (full_match, partial_match, any_match) + }, ); - false + let Some(transport_link_index) = full_match.map(|(i, _)| i).or(partial_match).or(any_match) + else { + tracing::trace!( + "Message dropped because the transport has no links: {}", + msg + ); + + // No Link found + return false; + }; + + let transport_link = transport_links + .get(transport_link_index) + .expect("transport link index should be valid"); + + let pipeline = transport_link.pipeline.clone(); + tracing::trace!( + "Scheduled {:?} for transmission to {} ({})", + msg, + transport_link.link.link.get_dst(), + self.get_zid() + ); + // Drop the guard before the push_zenoh_message since + // the link could be congested and this operation could + // block for fairly long time + drop(transport_links); + pipeline.push_network_message(msg) } #[allow(unused_mut)] // When feature "shared-memory" is not enabled From 5cfcf55d9cdad5bc475a41d652c45a0e3ba8091f Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Wed, 4 Sep 2024 08:38:06 +0000 Subject: [PATCH 02/37] Improve `QoS` state machine --- commons/zenoh-protocol/src/core/mod.rs | 8 +- .../src/unicast/establishment/ext/qos.rs | 91 +++++++++++++++---- .../src/unicast/universal/tx.rs | 2 +- 3 files changed, 79 insertions(+), 22 deletions(-) diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index 825da6eb5c..e484acdded 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -324,10 +324,16 @@ impl PriorityRange { Ok(Self { start, end }) } - pub fn includes(&self, priority: Priority) -> bool { + /// Returns `true` if `priority` is a member of `self`. + pub fn contains(&self, priority: Priority) -> bool { self.start <= (priority as u8) && (priority as u8) < self.end } + /// Returns `true` if `self` is a superset of `other`. + pub fn includes(&self, other: PriorityRange) -> bool { + self.start <= other.start && other.end <= self.end + } + pub fn len(&self) -> usize { (self.end - self.start) as usize } diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index 5bebe00bb0..fd3fa269b1 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -41,7 +41,7 @@ impl<'a> QoSFsm<'a> { } } -#[derive(Clone, Debug, PartialEq, Eq, Serialize)] +#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize)] pub(crate) enum QoS { Disabled, Enabled { priorities: Option }, @@ -117,23 +117,6 @@ impl QoS { } } - fn unify(&self, other: &Self) -> Self { - match (self, other) { - (QoS::Disabled, QoS::Disabled) => QoS::Disabled, - (QoS::Disabled, QoS::Enabled { priorities }) - | (QoS::Enabled { priorities }, QoS::Disabled) => QoS::Enabled { - priorities: *priorities, - }, - (QoS::Enabled { priorities: lhs }, QoS::Enabled { priorities: rhs }) => { - if lhs == rhs { - QoS::Enabled { priorities: *rhs } - } else { - QoS::Enabled { priorities: None } - } - } - } - } - pub(crate) fn is_enabled(&self) -> bool { matches!(self, QoS::Enabled { .. }) } @@ -217,7 +200,41 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { QoS::Disabled }; - *state_self = state_self.unify(&state_other); + *state_self = match (*state_self, state_other) { + (QoS::Disabled, _) | (_, QoS::Disabled) => QoS::Disabled, + (QoS::Enabled { priorities: None }, QoS::Enabled { priorities: None }) => { + QoS::Enabled { priorities: None } + } + ( + self_qos @ QoS::Enabled { + priorities: Some(_), + }, + QoS::Enabled { priorities: None }, + ) => self_qos, + ( + QoS::Enabled { priorities: None }, + other_qos @ QoS::Enabled { + priorities: Some(_), + }, + ) => other_qos, + ( + self_qos @ QoS::Enabled { + priorities: Some(priorities_self), + }, + QoS::Enabled { + priorities: Some(priorities_other), + }, + ) => { + if priorities_other.includes(priorities_self) { + self_qos + } else { + return Err(zerror!( + "The PriorityRange set in InitAck is not a superset of my PriorityRange" + ) + .into()); + } + } + }; Ok(()) } @@ -259,7 +276,41 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { QoS::Disabled }; - *state_self = state_self.unify(&state_other); + *state_self = match (*state_self, state_other) { + (QoS::Disabled, _) | (_, QoS::Disabled) => QoS::Disabled, + (QoS::Enabled { priorities: None }, QoS::Enabled { priorities: None }) => { + QoS::Enabled { priorities: None } + } + ( + self_qos @ QoS::Enabled { + priorities: Some(_), + }, + QoS::Enabled { priorities: None }, + ) => self_qos, + ( + QoS::Enabled { priorities: None }, + other_qos @ QoS::Enabled { + priorities: Some(_), + }, + ) => other_qos, + ( + QoS::Enabled { + priorities: Some(priorities_self), + }, + other_qos @ QoS::Enabled { + priorities: Some(priorities_other), + }, + ) => { + if priorities_self.includes(priorities_other) { + other_qos + } else { + return Err(zerror!( + "The PriorityRange set in InitSyn is not a subset of my PriorityRange" + ) + .into()); + } + } + }; Ok(()) } diff --git a/io/zenoh-transport/src/unicast/universal/tx.rs b/io/zenoh-transport/src/unicast/universal/tx.rs index 4fadcdb147..90cee7b981 100644 --- a/io/zenoh-transport/src/unicast/universal/tx.rs +++ b/io/zenoh-transport/src/unicast/universal/tx.rs @@ -38,7 +38,7 @@ impl TransportUnicastUniversal { .link .config .priorities - .and_then(|range| range.includes(msg.priority()).then_some(range)); + .and_then(|range| range.contains(msg.priority()).then_some(range)); match (reliability, priorities) { (true, Some(priorities)) => { From 1c9ee29ae49cdfc862d6e46496e3bb710ed66d9f Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Wed, 4 Sep 2024 09:17:49 +0000 Subject: [PATCH 03/37] Add `PriorityRange` negotiation tests --- .../src/unicast/establishment/ext/qos.rs | 204 +++++++++++++++--- io/zenoh-transport/src/unicast/link.rs | 2 +- 2 files changed, 174 insertions(+), 32 deletions(-) diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index fd3fa269b1..b094ad3999 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -107,7 +107,7 @@ impl QoS { /// 2. QoS is enabled but no priority range is available /// 3. QoS is enabled and priority information is range, in which case the next 16 least /// significant bits are used to encode the priority range. - fn to_u64(&self) -> u64 { + fn to_u64(self) -> u64 { match self { QoS::Disabled => 0b00_u64, QoS::Enabled { priorities: None } => 0b01_u64, @@ -117,6 +117,22 @@ impl QoS { } } + fn to_ext(self) -> Option { + if self.is_enabled() { + Some(init::ext::QoS::new(self.to_u64())) + } else { + None + } + } + + fn try_from_ext(ext: Option) -> ZResult { + if let Some(ext) = ext { + QoS::try_from_u64(ext.value) + } else { + Ok(QoS::Disabled) + } + } + pub(crate) fn is_enabled(&self) -> bool { matches!(self, QoS::Enabled { .. }) } @@ -179,11 +195,7 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { self, state: Self::SendInitSynIn, ) -> Result { - if state.is_enabled() { - Ok(Some(init::ext::QoS::new(state.to_u64()))) - } else { - Ok(None) - } + Ok(state.to_ext()) } type RecvInitAckIn = (&'a mut QoS, Option); @@ -194,11 +206,7 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { ) -> Result { let (state_self, other_ext) = input; - let state_other = if let Some(other_ext) = other_ext { - QoS::try_from_u64(other_ext.value)? - } else { - QoS::Disabled - }; + let state_other = QoS::try_from_ext(other_ext)?; *state_self = match (*state_self, state_other) { (QoS::Disabled, _) | (_, QoS::Disabled) => QoS::Disabled, @@ -206,17 +214,17 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { QoS::Enabled { priorities: None } } ( - self_qos @ QoS::Enabled { + qos @ QoS::Enabled { priorities: Some(_), }, QoS::Enabled { priorities: None }, - ) => self_qos, - ( + ) + | ( QoS::Enabled { priorities: None }, - other_qos @ QoS::Enabled { + qos @ QoS::Enabled { priorities: Some(_), }, - ) => other_qos, + ) => qos, ( self_qos @ QoS::Enabled { priorities: Some(priorities_self), @@ -270,11 +278,7 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { ) -> Result { let (state_self, other_ext) = input; - let state_other = if let Some(other_ext) = other_ext { - QoS::try_from_u64(other_ext.value)? - } else { - QoS::Disabled - }; + let state_other = QoS::try_from_ext(other_ext)?; *state_self = match (*state_self, state_other) { (QoS::Disabled, _) | (_, QoS::Disabled) => QoS::Disabled, @@ -282,17 +286,17 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { QoS::Enabled { priorities: None } } ( - self_qos @ QoS::Enabled { + qos @ QoS::Enabled { priorities: Some(_), }, QoS::Enabled { priorities: None }, - ) => self_qos, - ( + ) + | ( QoS::Enabled { priorities: None }, - other_qos @ QoS::Enabled { + qos @ QoS::Enabled { priorities: Some(_), }, - ) => other_qos, + ) => qos, ( QoS::Enabled { priorities: Some(priorities_self), @@ -321,11 +325,7 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { self, state: Self::SendInitAckIn, ) -> Result { - if state.is_enabled() { - Ok(Some(init::ext::QoS::new(state.to_u64()))) - } else { - Ok(None) - } + Ok(state.to_ext()) } type RecvOpenSynIn = (&'a mut QoS, Option); @@ -346,3 +346,145 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { Ok(None) } } + +#[cfg(test)] +mod tests { + use zenoh_protocol::core::PriorityRange; + use zenoh_result::ZResult; + + use crate::unicast::establishment::{AcceptFsm, OpenFsm}; + + use super::{QoS, QoSFsm}; + + async fn test_priority_range_negotiation( + qos_open: &mut QoS, + qos_accept: &mut QoS, + ) -> ZResult<()> { + let fsm = QoSFsm::new(); + + let ext = fsm.send_init_syn(&*qos_open).await?; + fsm.recv_init_syn((qos_accept, ext)).await?; + + let ext = fsm.send_init_ack(&*qos_accept).await?; + fsm.recv_init_ack((qos_open, ext)).await?; + + Ok(()) + } + + #[tokio::test] + async fn test_priority_range_negotiation_scenario_1() { + let qos_open = &mut QoS::Enabled { priorities: None }; + let qos_accept = &mut QoS::Enabled { priorities: None }; + + match test_priority_range_negotiation(qos_open, qos_accept).await { + Err(err) => panic!("expected `Ok(())`, got: {err}"), + Ok(()) => { + assert_eq!(*qos_open, *qos_accept); + assert_eq!(*qos_open, QoS::Enabled { priorities: None }); + } + }; + } + + #[tokio::test] + async fn test_priority_range_negotiation_scenario_2() { + let qos_open = &mut QoS::Enabled { priorities: None }; + let qos_accept = &mut QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + }; + + match test_priority_range_negotiation(qos_open, qos_accept).await { + Err(err) => panic!("expected `Ok(())`, got: {err}"), + Ok(()) => { + assert_eq!(*qos_open, *qos_accept); + assert_eq!( + *qos_open, + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()) + } + ); + } + }; + } + + #[tokio::test] + async fn test_priority_range_negotiation_scenario_3() { + let qos_open = &mut QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + }; + let qos_accept = &mut QoS::Enabled { priorities: None }; + + match test_priority_range_negotiation(qos_open, qos_accept).await { + Err(err) => panic!("expected `Ok(())`, got: {err}"), + Ok(()) => { + assert_eq!(*qos_open, *qos_accept); + assert_eq!( + *qos_open, + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()) + } + ); + } + }; + } + + #[tokio::test] + async fn test_priority_range_negotiation_scenario_4() { + let qos_open = &mut QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + }; + let qos_accept = &mut QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + }; + + match test_priority_range_negotiation(qos_open, qos_accept).await { + Err(err) => panic!("expected `Ok(())`, got: {err}"), + Ok(()) => { + assert_eq!(*qos_open, *qos_accept); + assert_eq!( + *qos_open, + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()) + } + ); + } + }; + } + + #[tokio::test] + async fn test_priority_range_negotiation_scenario_5() { + let qos_open = &mut QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + }; + let qos_accept = &mut QoS::Enabled { + priorities: Some(PriorityRange::new(0, 4).unwrap()), + }; + + match test_priority_range_negotiation(qos_open, qos_accept).await { + Err(err) => panic!("expected `Ok(())`, got: {err}"), + Ok(()) => { + assert_eq!(*qos_open, *qos_accept); + assert_eq!( + *qos_open, + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()) + } + ); + } + }; + } + + #[tokio::test] + async fn test_priority_range_negotiation_scenario_6() { + let qos_open = &mut QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + }; + let qos_accept = &mut QoS::Enabled { + priorities: Some(PriorityRange::new(2, 3).unwrap()), + }; + + match test_priority_range_negotiation(qos_open, qos_accept).await { + Ok(()) => panic!("expected `Err(_)`, got `Ok(())`"), + Err(_) => {} + }; + } +} diff --git a/io/zenoh-transport/src/unicast/link.rs b/io/zenoh-transport/src/unicast/link.rs index 6e8425f056..bfd69ff7dc 100644 --- a/io/zenoh-transport/src/unicast/link.rs +++ b/io/zenoh-transport/src/unicast/link.rs @@ -82,7 +82,7 @@ impl TransportLinkUnicast { pub(crate) fn rx(&self) -> TransportLinkUnicastRx { TransportLinkUnicastRx { link: self.link.clone(), - config: self.config.clone(), + config: self.config, } } From 162e356eb4e9a0de1843ccdcd05ec3fd700925eb Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Wed, 4 Sep 2024 09:55:41 +0000 Subject: [PATCH 04/37] Refactor link selection function --- .../src/unicast/establishment/ext/qos.rs | 3 +- .../src/unicast/universal/tx.rs | 84 +++++++++++-------- 2 files changed, 52 insertions(+), 35 deletions(-) diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index b094ad3999..69ff96dd84 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -352,9 +352,8 @@ mod tests { use zenoh_protocol::core::PriorityRange; use zenoh_result::ZResult; - use crate::unicast::establishment::{AcceptFsm, OpenFsm}; - use super::{QoS, QoSFsm}; + use crate::unicast::establishment::{AcceptFsm, OpenFsm}; async fn test_priority_range_negotiation( qos_open: &mut QoS, diff --git a/io/zenoh-transport/src/unicast/universal/tx.rs b/io/zenoh-transport/src/unicast/universal/tx.rs index 90cee7b981..129555ddbf 100644 --- a/io/zenoh-transport/src/unicast/universal/tx.rs +++ b/io/zenoh-transport/src/unicast/universal/tx.rs @@ -12,55 +12,77 @@ // ZettaScale Zenoh Team, // use zenoh_protocol::{ - core::{PriorityRange, Reliability}, + core::{Priority, PriorityRange, Reliability}, network::NetworkMessage, }; -use super::transport::TransportUnicastUniversal; +use super::{link::TransportLinkUnicastUniversal, transport::TransportUnicastUniversal}; #[cfg(feature = "shared-memory")] use crate::shm::map_zmsg_to_partner; use crate::unicast::transport_unicast_inner::TransportUnicastTrait; impl TransportUnicastUniversal { - fn schedule_on_link(&self, msg: NetworkMessage) -> bool { - let transport_links = self - .links - .read() - .expect("reading `TransportUnicastUniversal::links` should not fail"); - - let msg_reliability = Reliability::from(msg.is_reliable()); - + /// Returns the best matching [`TransportLinkUnicastUniversal`]. + /// + /// The result is either: + /// 1. A "full match" where the link matches both `reliability` and `priority`. In case of + /// multiple candidates, the link with the smaller range is selected. + /// 2. A "partial match" where the link match `reliability` and **not** `priority`. + /// 3. A "no match" where any available link is selected. + /// + /// If `transport_links` is empty then [`None`] is returned. + fn select_link( + transport_links: &[TransportLinkUnicastUniversal], + reliability: Reliability, + priority: Priority, + ) -> Option<&TransportLinkUnicastUniversal> { let (full_match, partial_match, any_match) = transport_links.iter().enumerate().fold( (None::<(_, PriorityRange)>, None, None), - |(mut full_match, mut partial_match, mut any_match), (i, transport_link)| { - let reliability = transport_link.link.config.reliability == msg_reliability; - let priorities = transport_link - .link - .config - .priorities - .and_then(|range| range.contains(msg.priority()).then_some(range)); + |(mut full_match, mut partial_match, mut no_match), (i, tl)| { + let config = tl.link.config; + + let reliability = config.reliability.eq(&reliability); + let priorities = config.priorities.filter(|range| range.contains(priority)); match (reliability, priorities) { - (true, Some(priorities)) => { - match full_match { - Some((_, r)) if priorities.len() < r.len() => { - full_match = Some((i, priorities)) - } - None => full_match = Some((i, priorities)), - _ => {} - }; + (true, Some(priorities)) + if full_match + .as_ref() + .map_or(true, |(_, r)| priorities.len() < r.len()) => + { + full_match = Some((i, priorities)) } (true, None) if partial_match.is_none() => partial_match = Some(i), - _ if any_match.is_none() => any_match = Some(i), + _ if no_match.is_none() => no_match = Some(i), _ => {} }; - (full_match, partial_match, any_match) + (full_match, partial_match, no_match) }, ); - let Some(transport_link_index) = full_match.map(|(i, _)| i).or(partial_match).or(any_match) - else { + full_match + .map(|(i, _)| i) + .or(partial_match) + .or(any_match) + .map(|i| { + transport_links + .get(i) + .expect("transport link index should be valid") + }) + } + + fn schedule_on_link(&self, msg: NetworkMessage) -> bool { + let transport_links = self + .links + .read() + .expect("reading `TransportUnicastUniversal::links` should not fail"); + + let Some(transport_link) = Self::select_link( + &transport_links, + Reliability::from(msg.is_reliable()), + msg.priority(), + ) else { tracing::trace!( "Message dropped because the transport has no links: {}", msg @@ -70,10 +92,6 @@ impl TransportUnicastUniversal { return false; }; - let transport_link = transport_links - .get(transport_link_index) - .expect("transport link index should be valid"); - let pipeline = transport_link.pipeline.clone(); tracing::trace!( "Scheduled {:?} for transmission to {} ({})", From 2f3b821e9cf55b6c260c3f49e2654513e9468933 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Wed, 4 Sep 2024 10:02:21 +0000 Subject: [PATCH 05/37] Minor edits --- io/zenoh-transport/src/unicast/establishment/ext/qos.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index 69ff96dd84..948594cc24 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -237,7 +237,7 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { self_qos } else { return Err(zerror!( - "The PriorityRange set in InitAck is not a superset of my PriorityRange" + "The PriorityRange received in InitAck is not a superset of my PriorityRange" ) .into()); } @@ -309,7 +309,7 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { other_qos } else { return Err(zerror!( - "The PriorityRange set in InitSyn is not a subset of my PriorityRange" + "The PriorityRange received in InitSyn is not a subset of my PriorityRange" ) .into()); } From 8b3eff12ca3a0b3ad36dd4a4032839edd510e3f3 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Wed, 4 Sep 2024 10:24:56 +0000 Subject: [PATCH 06/37] Add Link selectioh tests --- .../src/unicast/universal/tx.rs | 122 ++++++++++++++---- 1 file changed, 100 insertions(+), 22 deletions(-) diff --git a/io/zenoh-transport/src/unicast/universal/tx.rs b/io/zenoh-transport/src/unicast/universal/tx.rs index 129555ddbf..14b09f7f8e 100644 --- a/io/zenoh-transport/src/unicast/universal/tx.rs +++ b/io/zenoh-transport/src/unicast/universal/tx.rs @@ -16,13 +16,13 @@ use zenoh_protocol::{ network::NetworkMessage, }; -use super::{link::TransportLinkUnicastUniversal, transport::TransportUnicastUniversal}; +use super::transport::TransportUnicastUniversal; #[cfg(feature = "shared-memory")] use crate::shm::map_zmsg_to_partner; use crate::unicast::transport_unicast_inner::TransportUnicastTrait; impl TransportUnicastUniversal { - /// Returns the best matching [`TransportLinkUnicastUniversal`]. + /// Returns the index of the best matching [`Reliability`]-[`PriorityRange`] pair. /// /// The result is either: /// 1. A "full match" where the link matches both `reliability` and `priority`. In case of @@ -31,18 +31,16 @@ impl TransportUnicastUniversal { /// 3. A "no match" where any available link is selected. /// /// If `transport_links` is empty then [`None`] is returned. - fn select_link( - transport_links: &[TransportLinkUnicastUniversal], + fn select( + elements: impl Iterator)>, reliability: Reliability, priority: Priority, - ) -> Option<&TransportLinkUnicastUniversal> { - let (full_match, partial_match, any_match) = transport_links.iter().enumerate().fold( + ) -> Option { + let (full_match, partial_match, any_match) = elements.enumerate().fold( (None::<(_, PriorityRange)>, None, None), - |(mut full_match, mut partial_match, mut no_match), (i, tl)| { - let config = tl.link.config; - - let reliability = config.reliability.eq(&reliability); - let priorities = config.priorities.filter(|range| range.contains(priority)); + |(mut full_match, mut partial_match, mut no_match), (i, (reliability_, priorities))| { + let reliability = reliability_.eq(&reliability); + let priorities = priorities.filter(|range| range.contains(priority)); match (reliability, priorities) { (true, Some(priorities)) @@ -61,15 +59,7 @@ impl TransportUnicastUniversal { }, ); - full_match - .map(|(i, _)| i) - .or(partial_match) - .or(any_match) - .map(|i| { - transport_links - .get(i) - .expect("transport link index should be valid") - }) + full_match.map(|(i, _)| i).or(partial_match).or(any_match) } fn schedule_on_link(&self, msg: NetworkMessage) -> bool { @@ -78,8 +68,10 @@ impl TransportUnicastUniversal { .read() .expect("reading `TransportUnicastUniversal::links` should not fail"); - let Some(transport_link) = Self::select_link( - &transport_links, + let Some(transport_link_index) = Self::select( + transport_links + .iter() + .map(|tl| (tl.link.config.reliability, tl.link.config.priorities)), Reliability::from(msg.is_reliable()), msg.priority(), ) else { @@ -92,6 +84,10 @@ impl TransportUnicastUniversal { return false; }; + let transport_link = transport_links + .get(transport_link_index) + .expect("transport link index should be valid"); + let pipeline = transport_link.pipeline.clone(); tracing::trace!( "Scheduled {:?} for transmission to {} ({})", @@ -130,3 +126,85 @@ impl TransportUnicastUniversal { res } } + +#[cfg(test)] +mod tests { + use zenoh_protocol::core::{Priority, PriorityRange, Reliability}; + + use crate::unicast::universal::transport::TransportUnicastUniversal; + + #[test] + /// Tests the "full match" scenario with exactly one candidate. + fn test_link_selection_scenario_1() { + let selection = TransportUnicastUniversal::select( + [ + ( + Reliability::Reliable, + Some(PriorityRange::new(0, 1).unwrap()), + ), + ( + Reliability::Reliable, + Some(PriorityRange::new(1, 2).unwrap()), + ), + ( + Reliability::BestEffort, + Some(PriorityRange::new(0, 1).unwrap()), + ), + ] + .into_iter(), + Reliability::Reliable, + Priority::try_from(0).unwrap(), + ); + assert_eq!(selection, Some(0)); + } + + #[test] + /// Tests the "full match" scenario with multiple candidates. + fn test_link_selection_scenario_2() { + let selection = TransportUnicastUniversal::select( + [ + ( + Reliability::Reliable, + Some(PriorityRange::new(0, 2).unwrap()), + ), + ( + Reliability::Reliable, + Some(PriorityRange::new(0, 1).unwrap()), + ), + ] + .into_iter(), + Reliability::Reliable, + Priority::try_from(0).unwrap(), + ); + assert_eq!(selection, Some(1)); + } + + #[test] + /// Tests the "partial match" scenario. + fn test_link_selection_scenario_3() { + let selection = TransportUnicastUniversal::select( + [ + ( + Reliability::BestEffort, + Some(PriorityRange::new(0, 1).unwrap()), + ), + (Reliability::Reliable, None), + ] + .into_iter(), + Reliability::Reliable, + Priority::try_from(0).unwrap(), + ); + assert_eq!(selection, Some(1)); + } + + #[test] + /// Tests the "no match" scenario. + fn test_link_selection_scenario_4() { + let selection = TransportUnicastUniversal::select( + [(Reliability::BestEffort, None)].into_iter(), + Reliability::Reliable, + Priority::try_from(0).unwrap(), + ); + assert_eq!(selection, Some(0)); + } +} From 22e78073e5caae9e684cef42ce26164394f48035 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Wed, 4 Sep 2024 10:38:56 +0000 Subject: [PATCH 07/37] Minor edits --- .../src/unicast/universal/tx.rs | 35 +++++++++++-------- 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/io/zenoh-transport/src/unicast/universal/tx.rs b/io/zenoh-transport/src/unicast/universal/tx.rs index 14b09f7f8e..70b8a72bf3 100644 --- a/io/zenoh-transport/src/unicast/universal/tx.rs +++ b/io/zenoh-transport/src/unicast/universal/tx.rs @@ -28,7 +28,7 @@ impl TransportUnicastUniversal { /// 1. A "full match" where the link matches both `reliability` and `priority`. In case of /// multiple candidates, the link with the smaller range is selected. /// 2. A "partial match" where the link match `reliability` and **not** `priority`. - /// 3. A "no match" where any available link is selected. + /// 3. An "any match" where any available link is selected. /// /// If `transport_links` is empty then [`None`] is returned. fn select( @@ -36,30 +36,35 @@ impl TransportUnicastUniversal { reliability: Reliability, priority: Priority, ) -> Option { - let (full_match, partial_match, any_match) = elements.enumerate().fold( - (None::<(_, PriorityRange)>, None, None), - |(mut full_match, mut partial_match, mut no_match), (i, (reliability_, priorities))| { - let reliability = reliability_.eq(&reliability); - let priorities = priorities.filter(|range| range.contains(priority)); + #[derive(Default)] + struct Match { + full: Option, + partial: Option, + any: Option, + } - match (reliability, priorities) { + let (match_, _) = elements.enumerate().fold( + (Match::default(), Option::::None), + |(mut match_, mut prev_priorities), (i, (r, ps))| { + match (r.eq(&reliability), ps.filter(|ps| ps.contains(priority))) { (true, Some(priorities)) - if full_match + if prev_priorities .as_ref() - .map_or(true, |(_, r)| priorities.len() < r.len()) => + .map_or(true, |ps| ps.len() > priorities.len()) => { - full_match = Some((i, priorities)) + match_.full = Some(i); + prev_priorities = Some(priorities); } - (true, None) if partial_match.is_none() => partial_match = Some(i), - _ if no_match.is_none() => no_match = Some(i), + (true, None) if match_.partial.is_none() => match_.partial = Some(i), + _ if match_.any.is_none() => match_.any = Some(i), _ => {} }; - (full_match, partial_match, no_match) + (match_, prev_priorities) }, ); - full_match.map(|(i, _)| i).or(partial_match).or(any_match) + match_.full.or(match_.partial).or(match_.any) } fn schedule_on_link(&self, msg: NetworkMessage) -> bool { @@ -198,7 +203,7 @@ mod tests { } #[test] - /// Tests the "no match" scenario. + /// Tests the "any match" scenario. fn test_link_selection_scenario_4() { let selection = TransportUnicastUniversal::select( [(Reliability::BestEffort, None)].into_iter(), From 3c9af4ca3f838e24f06f9ab4c21261a5e6bfb604 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Wed, 4 Sep 2024 10:44:54 +0000 Subject: [PATCH 08/37] More minor edits --- io/zenoh-transport/src/unicast/universal/tx.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/io/zenoh-transport/src/unicast/universal/tx.rs b/io/zenoh-transport/src/unicast/universal/tx.rs index 70b8a72bf3..ccbf9fecba 100644 --- a/io/zenoh-transport/src/unicast/universal/tx.rs +++ b/io/zenoh-transport/src/unicast/universal/tx.rs @@ -25,12 +25,12 @@ impl TransportUnicastUniversal { /// Returns the index of the best matching [`Reliability`]-[`PriorityRange`] pair. /// /// The result is either: - /// 1. A "full match" where the link matches both `reliability` and `priority`. In case of - /// multiple candidates, the link with the smaller range is selected. - /// 2. A "partial match" where the link match `reliability` and **not** `priority`. - /// 3. An "any match" where any available link is selected. + /// 1. A "full match" where the pair matches both `reliability` and `priority`. In case of + /// multiple candidates, the pair with the smaller range is selected. + /// 2. A "partial match" where the pair match `reliability` and **not** `priority`. + /// 3. An "any match" where any available pair is selected. /// - /// If `transport_links` is empty then [`None`] is returned. + /// If `elements` is empty then [`None`] is returned. fn select( elements: impl Iterator)>, reliability: Reliability, From 20a61691fc3500ef0acfb278bcb1bd4c1c5366a9 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Wed, 4 Sep 2024 10:50:09 +0000 Subject: [PATCH 09/37] Never disobey Clippy --- io/zenoh-transport/src/unicast/establishment/ext/qos.rs | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index 948594cc24..18f05262a9 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -481,9 +481,8 @@ mod tests { priorities: Some(PriorityRange::new(2, 3).unwrap()), }; - match test_priority_range_negotiation(qos_open, qos_accept).await { - Ok(()) => panic!("expected `Err(_)`, got `Ok(())`"), - Err(_) => {} - }; + if let Ok(()) = test_priority_range_negotiation(qos_open, qos_accept).await { + panic!("expected `Err(_)`, got `Ok(())`") + } } } From 8ac3ea83737667ce88ec981f970713cfe507c399 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Wed, 4 Sep 2024 16:21:56 +0000 Subject: [PATCH 10/37] Implement Reliability negotiation --- commons/zenoh-protocol/src/core/mod.rs | 51 +- .../src/unicast/establishment/ext/qos.rs | 481 +++++++++++++----- 2 files changed, 413 insertions(+), 119 deletions(-) diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index e484acdded..3307bcfa4a 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -19,10 +19,11 @@ use alloc::{ }; use core::{ convert::{From, TryFrom, TryInto}, - fmt, + fmt::{self, Display}, hash::Hash, str::FromStr, }; +use std::error::Error; use serde::Serialize; pub use uhlc::{Timestamp, NTP64}; @@ -406,6 +407,14 @@ pub enum Reliability { impl Reliability { pub const DEFAULT: Self = Self::Reliable; + /// Returns `true` is `self` implies `other`. + pub fn implies(self, other: Self) -> bool { + match (self, other) { + (Reliability::Reliable, Reliability::BestEffort) => false, + _ => true, + } + } + #[cfg(feature = "test")] pub fn rand() -> Self { use rand::Rng; @@ -430,6 +439,46 @@ impl From for Reliability { } } +impl From for bool { + fn from(value: Reliability) -> Self { + match value { + Reliability::BestEffort => false, + Reliability::Reliable => true, + } + } +} + +#[derive(Debug)] +pub struct InvalidReliability { + found: String, +} + +impl Display for InvalidReliability { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + f, + "invalid Reliability string, expected `best_effort` or `reliable` but found {}", + self.found + ) + } +} + +impl Error for InvalidReliability {} + +impl FromStr for Reliability { + type Err = InvalidReliability; + + fn from_str(s: &str) -> Result { + match s { + "reliable" => Ok(Reliability::Reliable), + "best_effort" => Ok(Reliability::BestEffort), + other => Err(InvalidReliability { + found: other.to_string(), + }), + } + } +} + #[derive(Debug, Copy, Clone, PartialEq, Eq, Default)] pub struct Channel { pub priority: Priority, diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index 18f05262a9..e959f23d53 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -12,6 +12,7 @@ // ZettaScale Zenoh Team, // use core::marker::PhantomData; +use std::str::FromStr; use async_trait::async_trait; use serde::Serialize; @@ -23,7 +24,7 @@ use zenoh_codec::{RCodec, WCodec, Zenoh080}; use zenoh_core::{bail, zerror}; use zenoh_link::EndPoint; use zenoh_protocol::{ - core::PriorityRange, + core::{PriorityRange, Reliability}, transport::{init, open}, }; use zenoh_result::{Error as ZError, ZResult}; @@ -44,55 +45,87 @@ impl<'a> QoSFsm<'a> { #[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize)] pub(crate) enum QoS { Disabled, - Enabled { priorities: Option }, + Enabled { + reliability: Option, + priorities: Option, + }, } impl QoS { pub(crate) fn new(is_qos: bool, endpoint: &EndPoint) -> ZResult { if !is_qos { - Ok(Self::Disabled) + Ok(QoS::Disabled) } else { + const RELIABILITY_METADATA_KEY: &str = "reliability"; const PRIORITY_METADATA_KEY: &str = "priorities"; - let endpoint_metadata = endpoint.metadata(); + let metadata = endpoint.metadata(); + + let reliability = metadata + .get(RELIABILITY_METADATA_KEY) + .map(Reliability::from_str) + .transpose()?; - let Some(mut priorities) = endpoint_metadata + let priorities = metadata .get(PRIORITY_METADATA_KEY) - .map(|metadata| metadata.split("..")) - else { - return Ok(Self::Enabled { priorities: None }); - }; - let start = priorities - .next() - .ok_or(zerror!("Invalid priority range syntax"))? - .parse::()?; - - let end = priorities - .next() - .ok_or(zerror!("Invalid priority range syntax"))? - .parse::()?; - - if priorities.next().is_some() { - bail!("Invalid priority range syntax") - }; - - Ok(Self::Enabled { - priorities: Some(PriorityRange::new(start, end)?), + .map(|metadata| { + let mut metadata = metadata.split(".."); + + let start = metadata + .next() + .ok_or(zerror!("Invalid priority range syntax"))? + .parse::()?; + + let end = metadata + .next() + .ok_or(zerror!("Invalid priority range syntax"))? + .parse::()?; + + if metadata.next().is_some() { + bail!("Invalid priority range syntax") + }; + + PriorityRange::new(start, end) + }) + .transpose()?; + + Ok(QoS::Enabled { + priorities, + reliability, }) } } fn try_from_u64(value: u64) -> ZResult { match value { - 0b00_u64 => Ok(QoS::Disabled), - 0b01_u64 => Ok(QoS::Enabled { priorities: None }), - mut value if value & 0b10_u64 != 0 => { - value >>= 2; - let start = (value & 0xff) as u8; - let end = ((value & 0xff00) >> 8) as u8; + 0b000_u64 => Ok(QoS::Disabled), + 0b001_u64 => Ok(QoS::Enabled { + priorities: None, + reliability: None, + }), + value if value & 0b110_u64 != 0 => { + let tag = value & 0b111_u64; + + let priorities = if tag & 0b010_u64 != 0 { + let start = ((value >> 3) & 0xff) as u8; + let end = ((value >> (3 + 8)) & 0xff) as u8; + + Some(PriorityRange::new(start, end)?) + } else { + None + }; + + let reliability = if tag & 0b100_u64 != 0 { + let bit = ((value >> (3 + 8 + 8)) & 0x1) as u8 == 1; + + Some(Reliability::from(bit)) + } else { + None + }; Ok(QoS::Enabled { - priorities: Some(PriorityRange::new(start, end)?), + priorities, + reliability, }) } _ => Err(zerror!("invalid QoS").into()), @@ -101,19 +134,39 @@ impl QoS { /// Encodes [`QoS`] as a [`u64`]. /// - /// The two least significant bits are used to discrimnate three states: + /// The three least significant bits are used to discrimnate five states: /// /// 1. QoS is disabled - /// 2. QoS is enabled but no priority range is available - /// 3. QoS is enabled and priority information is range, in which case the next 16 least - /// significant bits are used to encode the priority range. + /// 2. QoS is enabled but no priority range and no reliability setting are available + /// 3. QoS is enabled and priority range is available but reliability is unavailable + /// 4. QoS is enabled and reliability is available but priority range is unavailable + /// 5. QoS is enabled and both priority range and reliability are available fn to_u64(self) -> u64 { match self { - QoS::Disabled => 0b00_u64, - QoS::Enabled { priorities: None } => 0b01_u64, + QoS::Disabled => 0b000_u64, QoS::Enabled { - priorities: Some(range), - } => ((range.end() as u64) << 10) | ((range.start() as u64) << 2) | 0b10_u64, + priorities, + reliability, + } => { + if reliability.is_none() && priorities.is_none() { + return 0b001_u64; + } + + let mut value = 0b000_u64; + + if let Some(priorities) = priorities { + value |= 0b010_u64; + value |= (priorities.start() as u64) << 3; + value |= (priorities.end() as u64) << (3 + 8); + } + + if let Some(reliability) = reliability { + value |= 0b100_u64; + value |= (bool::from(reliability) as u64) << (3 + 8 + 8); + } + + value + } } } @@ -139,24 +192,45 @@ impl QoS { pub(crate) fn priorities(&self) -> Option { match self { - QoS::Disabled | QoS::Enabled { priorities: None } => None, + QoS::Disabled + | QoS::Enabled { + priorities: None, .. + } => None, QoS::Enabled { priorities: Some(priorities), + .. } => Some(*priorities), } } + pub(crate) fn reliability(&self) -> Option { + match self { + QoS::Disabled + | QoS::Enabled { + reliability: None, .. + } => None, + QoS::Enabled { + reliability: Some(reliability), + .. + } => Some(*reliability), + } + } + #[cfg(test)] pub(crate) fn rand() -> Self { use rand::Rng; let mut rng = rand::thread_rng(); if rng.gen_bool(0.5) { QoS::Disabled - } else if rng.gen_bool(0.5) { - QoS::Enabled { priorities: None } } else { + let priorities = rng.gen_bool(0.5).then(|| PriorityRange::rand()); + let reliability = rng + .gen_bool(0.5) + .then(|| Reliability::from(rng.gen_bool(0.5))); + QoS::Enabled { - priorities: Some(PriorityRange::rand()), + priorities, + reliability, } } } @@ -208,33 +282,26 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { let state_other = QoS::try_from_ext(other_ext)?; - *state_self = match (*state_self, state_other) { - (QoS::Disabled, _) | (_, QoS::Disabled) => QoS::Disabled, - (QoS::Enabled { priorities: None }, QoS::Enabled { priorities: None }) => { - QoS::Enabled { priorities: None } - } - ( - qos @ QoS::Enabled { - priorities: Some(_), - }, - QoS::Enabled { priorities: None }, - ) - | ( - QoS::Enabled { priorities: None }, - qos @ QoS::Enabled { - priorities: Some(_), - }, - ) => qos, - ( - self_qos @ QoS::Enabled { - priorities: Some(priorities_self), - }, - QoS::Enabled { - priorities: Some(priorities_other), - }, - ) => { - if priorities_other.includes(priorities_self) { - self_qos + let ( + QoS::Enabled { + reliability: self_reliability, + priorities: self_priorities, + }, + QoS::Enabled { + reliability: other_reliability, + priorities: other_priorities, + }, + ) = (*state_self, state_other) + else { + *state_self = QoS::Disabled; + return Ok(()); + }; + + let priorities = match (self_priorities, other_priorities) { + (None, priorities) | (priorities, None) => priorities, + (Some(self_priorities), Some(other_priorities)) => { + if other_priorities.includes(self_priorities) { + Some(self_priorities) } else { return Err(zerror!( "The PriorityRange received in InitAck is not a superset of my PriorityRange" @@ -244,6 +311,25 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { } }; + let reliability = match (self_reliability, other_reliability) { + (None, reliability) | (reliability, None) => reliability, + (Some(self_reliability), Some(other_reliability)) => { + if other_reliability.implies(self_reliability) { + Some(self_reliability) + } else { + return Err(zerror!( + "The Reliability received in InitAck cannot be substituted with my Reliability" + ) + .into()); + } + } + }; + + *state_self = QoS::Enabled { + reliability, + priorities, + }; + Ok(()) } @@ -280,33 +366,26 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { let state_other = QoS::try_from_ext(other_ext)?; - *state_self = match (*state_self, state_other) { - (QoS::Disabled, _) | (_, QoS::Disabled) => QoS::Disabled, - (QoS::Enabled { priorities: None }, QoS::Enabled { priorities: None }) => { - QoS::Enabled { priorities: None } - } - ( - qos @ QoS::Enabled { - priorities: Some(_), - }, - QoS::Enabled { priorities: None }, - ) - | ( - QoS::Enabled { priorities: None }, - qos @ QoS::Enabled { - priorities: Some(_), - }, - ) => qos, - ( - QoS::Enabled { - priorities: Some(priorities_self), - }, - other_qos @ QoS::Enabled { - priorities: Some(priorities_other), - }, - ) => { - if priorities_self.includes(priorities_other) { - other_qos + let ( + QoS::Enabled { + reliability: self_reliability, + priorities: self_priorities, + }, + QoS::Enabled { + reliability: other_reliability, + priorities: other_priorities, + }, + ) = (*state_self, state_other) + else { + *state_self = QoS::Disabled; + return Ok(()); + }; + + let priorities = match (self_priorities, other_priorities) { + (None, priorities) | (priorities, None) => priorities, + (Some(self_priorities), Some(other_priorities)) => { + if self_priorities.includes(other_priorities) { + Some(other_priorities) } else { return Err(zerror!( "The PriorityRange received in InitSyn is not a subset of my PriorityRange" @@ -316,6 +395,25 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { } }; + let reliability = match (self_reliability, other_reliability) { + (None, reliability) | (reliability, None) => reliability, + (Some(self_reliability), Some(other_reliability)) => { + if self_reliability.implies(other_reliability) { + Some(other_reliability) + } else { + return Err(zerror!( + "The Reliability received in InitSyn cannot be substituted for my Reliability" + ) + .into()); + } + } + }; + + *state_self = QoS::Enabled { + reliability, + priorities, + }; + Ok(()) } @@ -349,16 +447,13 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { #[cfg(test)] mod tests { - use zenoh_protocol::core::PriorityRange; + use zenoh_protocol::core::{PriorityRange, Reliability}; use zenoh_result::ZResult; use super::{QoS, QoSFsm}; use crate::unicast::establishment::{AcceptFsm, OpenFsm}; - async fn test_priority_range_negotiation( - qos_open: &mut QoS, - qos_accept: &mut QoS, - ) -> ZResult<()> { + async fn test_negotiation(qos_open: &mut QoS, qos_accept: &mut QoS) -> ZResult<()> { let fsm = QoSFsm::new(); let ext = fsm.send_init_syn(&*qos_open).await?; @@ -372,33 +467,50 @@ mod tests { #[tokio::test] async fn test_priority_range_negotiation_scenario_1() { - let qos_open = &mut QoS::Enabled { priorities: None }; - let qos_accept = &mut QoS::Enabled { priorities: None }; + let qos_open = &mut QoS::Enabled { + priorities: None, + reliability: None, + }; + let qos_accept = &mut QoS::Enabled { + priorities: None, + reliability: None, + }; - match test_priority_range_negotiation(qos_open, qos_accept).await { + match test_negotiation(qos_open, qos_accept).await { Err(err) => panic!("expected `Ok(())`, got: {err}"), Ok(()) => { assert_eq!(*qos_open, *qos_accept); - assert_eq!(*qos_open, QoS::Enabled { priorities: None }); + assert_eq!( + *qos_open, + QoS::Enabled { + priorities: None, + reliability: None + } + ); } }; } #[tokio::test] async fn test_priority_range_negotiation_scenario_2() { - let qos_open = &mut QoS::Enabled { priorities: None }; + let qos_open = &mut QoS::Enabled { + priorities: None, + reliability: None, + }; let qos_accept = &mut QoS::Enabled { priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, }; - match test_priority_range_negotiation(qos_open, qos_accept).await { + match test_negotiation(qos_open, qos_accept).await { Err(err) => panic!("expected `Ok(())`, got: {err}"), Ok(()) => { assert_eq!(*qos_open, *qos_accept); assert_eq!( *qos_open, QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()) + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None } ); } @@ -409,17 +521,22 @@ mod tests { async fn test_priority_range_negotiation_scenario_3() { let qos_open = &mut QoS::Enabled { priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }; + let qos_accept = &mut QoS::Enabled { + priorities: None, + reliability: None, }; - let qos_accept = &mut QoS::Enabled { priorities: None }; - match test_priority_range_negotiation(qos_open, qos_accept).await { + match test_negotiation(qos_open, qos_accept).await { Err(err) => panic!("expected `Ok(())`, got: {err}"), Ok(()) => { assert_eq!(*qos_open, *qos_accept); assert_eq!( *qos_open, QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()) + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None } ); } @@ -430,19 +547,22 @@ mod tests { async fn test_priority_range_negotiation_scenario_4() { let qos_open = &mut QoS::Enabled { priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, }; let qos_accept = &mut QoS::Enabled { priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, }; - match test_priority_range_negotiation(qos_open, qos_accept).await { + match test_negotiation(qos_open, qos_accept).await { Err(err) => panic!("expected `Ok(())`, got: {err}"), Ok(()) => { assert_eq!(*qos_open, *qos_accept); assert_eq!( *qos_open, QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()) + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None } ); } @@ -453,19 +573,22 @@ mod tests { async fn test_priority_range_negotiation_scenario_5() { let qos_open = &mut QoS::Enabled { priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, }; let qos_accept = &mut QoS::Enabled { priorities: Some(PriorityRange::new(0, 4).unwrap()), + reliability: None, }; - match test_priority_range_negotiation(qos_open, qos_accept).await { + match test_negotiation(qos_open, qos_accept).await { Err(err) => panic!("expected `Ok(())`, got: {err}"), Ok(()) => { assert_eq!(*qos_open, *qos_accept); assert_eq!( *qos_open, QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()) + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None } ); } @@ -476,13 +599,135 @@ mod tests { async fn test_priority_range_negotiation_scenario_6() { let qos_open = &mut QoS::Enabled { priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, }; let qos_accept = &mut QoS::Enabled { priorities: Some(PriorityRange::new(2, 3).unwrap()), + reliability: None, }; - if let Ok(()) = test_priority_range_negotiation(qos_open, qos_accept).await { + if let Ok(()) = test_negotiation(qos_open, qos_accept).await { panic!("expected `Err(_)`, got `Ok(())`") } } + + #[tokio::test] + async fn test_reliability_negotiation_scenario_2() { + let qos_open = &mut QoS::Enabled { + reliability: None, + priorities: None, + }; + let qos_accept = &mut QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: None, + }; + + match test_negotiation(qos_open, qos_accept).await { + Err(err) => panic!("expected `Ok(())`, got: {err}"), + Ok(()) => { + assert_eq!(*qos_open, *qos_accept); + assert_eq!( + *qos_open, + QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: None + } + ); + } + }; + } + + #[tokio::test] + async fn test_reliability_negotiation_scenario_3() { + let qos_open = &mut QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None, + }; + let qos_accept = &mut QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: None, + }; + + match test_negotiation(qos_open, qos_accept).await { + Err(err) => panic!("expected `Ok(())`, got: {err}"), + Ok(()) => { + assert_eq!(*qos_open, *qos_accept); + assert_eq!( + *qos_open, + QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None + } + ); + } + }; + } + + #[tokio::test] + async fn test_reliability_negotiation_scenario_4() { + let qos_open = &mut QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None, + }; + let qos_accept = &mut QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None, + }; + + match test_negotiation(qos_open, qos_accept).await { + Err(err) => panic!("expected `Ok(())`, got: {err}"), + Ok(()) => { + assert_eq!(*qos_open, *qos_accept); + assert_eq!( + *qos_open, + QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None + } + ); + } + }; + } + + #[tokio::test] + async fn test_reliability_negotiation_scenario_5() { + let qos_open = &mut QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: None, + }; + let qos_accept = &mut QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None, + }; + + if let Ok(()) = test_negotiation(qos_open, qos_accept).await { + panic!("expected `Err(_)`, got `Ok(())`") + } + } + + #[tokio::test] + async fn test_priority_range_and_reliability_negotiation_scenario_1() { + let qos_open = &mut QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: Some(PriorityRange::new(1, 3).unwrap()), + }; + let qos_accept = &mut QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: Some(PriorityRange::new(1, 4).unwrap()), + }; + + match test_negotiation(qos_open, qos_accept).await { + Err(err) => panic!("expected `Ok(())`, got: {err}"), + Ok(()) => { + assert_eq!(*qos_open, *qos_accept); + assert_eq!( + *qos_open, + QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: Some(PriorityRange::new(1, 3).unwrap()) + } + ); + } + }; + } } From af43d88dd9f9ad27bbbc9f25fadc6f1fe05601ef Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Thu, 5 Sep 2024 07:33:53 +0000 Subject: [PATCH 11/37] Apply negotiated Reliability to Link config --- io/zenoh-transport/src/unicast/establishment/accept.rs | 6 +++++- io/zenoh-transport/src/unicast/establishment/open.rs | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/io/zenoh-transport/src/unicast/establishment/accept.rs b/io/zenoh-transport/src/unicast/establishment/accept.rs index 711fb000e4..94baee52fe 100644 --- a/io/zenoh-transport/src/unicast/establishment/accept.rs +++ b/io/zenoh-transport/src/unicast/establishment/accept.rs @@ -796,7 +796,11 @@ pub(crate) async fn accept_link( is_compression: state.link.ext_compression.is_compression(), }, priorities: state.transport.ext_qos.priorities(), - reliability: Reliability::from(link.link.is_reliable()), + reliability: state + .transport + .ext_qos + .reliability() + .unwrap_or_else(|| Reliability::from(link.link.is_reliable())), }; let a_link = link.reconfigure(a_config); let s_link = format!("{:?}", a_link); diff --git a/io/zenoh-transport/src/unicast/establishment/open.rs b/io/zenoh-transport/src/unicast/establishment/open.rs index 7d885c3139..81b8ed90a9 100644 --- a/io/zenoh-transport/src/unicast/establishment/open.rs +++ b/io/zenoh-transport/src/unicast/establishment/open.rs @@ -676,7 +676,11 @@ pub(crate) async fn open_link( is_compression: state.link.ext_compression.is_compression(), }, priorities: state.transport.ext_qos.priorities(), - reliability: Reliability::from(link.link.is_reliable()), + reliability: state + .transport + .ext_qos + .reliability() + .unwrap_or_else(|| Reliability::from(link.link.is_reliable())), }; let o_link = link.reconfigure(o_config); let s_link = format!("{:?}", o_link); From 7e26110747d5cf6d7dc1b34b98900d2c22d3bd43 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Thu, 5 Sep 2024 07:48:13 +0000 Subject: [PATCH 12/37] Document Endpoint `reliability` metadata --- DEFAULT_CONFIG.json5 | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/DEFAULT_CONFIG.json5 b/DEFAULT_CONFIG.json5 index d761f3f763..f70082559f 100644 --- a/DEFAULT_CONFIG.json5 +++ b/DEFAULT_CONFIG.json5 @@ -337,11 +337,16 @@ /// If not configured, all the supported protocols are automatically whitelisted. /// The supported protocols are: ["tcp" , "udp", "tls", "quic", "ws", "unixsock-stream", "vsock"] /// For example, to only enable "tls" and "quic": - // protocols: ["tls", "quic"], + /// protocols: ["tls", "quic"], /// - /// Endpoints accept a "priorities" metadata value in the form of a Rust-style half-open range - /// (e.g. `2..4` signifies priorities 2 and 3). This value is used to select the link used - /// for transmission based on the priority of the message in question. + /// ## Endpoint metadata + /// + /// **priorities**: a Rust-style half-open range (e.g. `2..4` signifies priorities 2 and 3). + /// This value is used to select the link used for transmission based on the Priority of the + /// message in question. + /// + /// **reliability**: either "best_effort" or "reliable". This value is used to select the link + /// used for transmission based on the Reliability of the message in question. /// /// Configure the zenoh TX parameters of a link tx: { @@ -399,7 +404,7 @@ enabled: true, /// The maximum time limit (in ms) a message should be retained for batching when back-pressure happens. time_limit: 1, - } + }, }, }, /// Configure the zenoh RX parameters of a link From b559fa7c0e5f37b3ce3cea42b7d3feae1a56294a Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Thu, 5 Sep 2024 07:59:36 +0000 Subject: [PATCH 13/37] I'm sorry Clippy --- commons/zenoh-protocol/src/core/mod.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index 3307bcfa4a..83379b0b0f 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -409,10 +409,10 @@ impl Reliability { /// Returns `true` is `self` implies `other`. pub fn implies(self, other: Self) -> bool { - match (self, other) { - (Reliability::Reliable, Reliability::BestEffort) => false, - _ => true, - } + !matches!( + (self, other), + (Reliability::Reliable, Reliability::BestEffort) + ) } #[cfg(feature = "test")] From e98fb4f7ffb3850919304d799eb8e5a091effdcc Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Thu, 5 Sep 2024 12:12:48 +0000 Subject: [PATCH 14/37] Make `PriorityRange` inclusive --- DEFAULT_CONFIG.json5 | 15 +++++++-------- commons/zenoh-protocol/src/core/mod.rs | 7 ++++--- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/DEFAULT_CONFIG.json5 b/DEFAULT_CONFIG.json5 index f70082559f..90eda6d324 100644 --- a/DEFAULT_CONFIG.json5 +++ b/DEFAULT_CONFIG.json5 @@ -333,17 +333,16 @@ }, }, link: { - /// An optional whitelist of protocols to be used for accepting and opening sessions. - /// If not configured, all the supported protocols are automatically whitelisted. - /// The supported protocols are: ["tcp" , "udp", "tls", "quic", "ws", "unixsock-stream", "vsock"] - /// For example, to only enable "tls" and "quic": - /// protocols: ["tls", "quic"], + /// An optional whitelist of protocols to be used for accepting and opening sessions. If not + /// configured, all the supported protocols are automatically whitelisted. The supported + /// protocols are: ["tcp" , "udp", "tls", "quic", "ws", "unixsock-stream", "vsock"] For + /// example, to only enable "tls" and "quic": protocols: ["tls", "quic"], /// /// ## Endpoint metadata /// - /// **priorities**: a Rust-style half-open range (e.g. `2..4` signifies priorities 2 and 3). - /// This value is used to select the link used for transmission based on the Priority of the - /// message in question. + /// **priorities**: a range bounded inclusively below and above (e.g. `2..4` signifies + /// priorities 2, 3 and 4). This value is used to select the link used for transmission based + /// on the Priority of the message in question. /// /// **reliability**: either "best_effort" or "reliable". This value is used to select the link /// used for transmission based on the Reliability of the message in question. diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index 83379b0b0f..137beaa382 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -311,6 +311,7 @@ pub enum Priority { } #[derive(Debug, Default, Copy, Clone, Eq, Hash, PartialEq, Serialize)] +/// A `u8` range bounded inclusively below and above. pub struct PriorityRange { pub start: u8, pub end: u8, @@ -318,7 +319,7 @@ pub struct PriorityRange { impl PriorityRange { pub fn new(start: u8, end: u8) -> ZResult { - if start >= end || start < Priority::MAX as u8 || end > Priority::MIN as u8 + 1 { + if start > end || start < Priority::MAX as u8 || end > Priority::MIN as u8 { bail!("Invalid priority range: {start}..{end}") }; @@ -327,7 +328,7 @@ impl PriorityRange { /// Returns `true` if `priority` is a member of `self`. pub fn contains(&self, priority: Priority) -> bool { - self.start <= (priority as u8) && (priority as u8) < self.end + self.start <= (priority as u8) && (priority as u8) <= self.end } /// Returns `true` if `self` is a superset of `other`. @@ -336,7 +337,7 @@ impl PriorityRange { } pub fn len(&self) -> usize { - (self.end - self.start) as usize + (self.end - self.start + 1) as usize } pub fn is_empty(&self) -> bool { From 02a6cbaf48ff8667a1878b7c1d3ada54d52bba3a Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Thu, 5 Sep 2024 12:15:22 +0000 Subject: [PATCH 15/37] Clippy lints are inevitable --- io/zenoh-transport/src/unicast/establishment/ext/qos.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index e959f23d53..7e0de9cf6a 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -223,7 +223,7 @@ impl QoS { if rng.gen_bool(0.5) { QoS::Disabled } else { - let priorities = rng.gen_bool(0.5).then(|| PriorityRange::rand()); + let priorities = rng.gen_bool(0.5).then(PriorityRange::rand); let reliability = rng .gen_bool(0.5) .then(|| Reliability::from(rng.gen_bool(0.5))); From 1758b204dcd71ae6d212d8435f3d5130040ac42a Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Thu, 5 Sep 2024 12:29:08 +0000 Subject: [PATCH 16/37] Make Reliability negotiation stricter --- commons/zenoh-protocol/src/core/mod.rs | 8 -------- io/zenoh-transport/src/unicast/establishment/ext/qos.rs | 8 ++++---- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index 137beaa382..d7ba72e850 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -408,14 +408,6 @@ pub enum Reliability { impl Reliability { pub const DEFAULT: Self = Self::Reliable; - /// Returns `true` is `self` implies `other`. - pub fn implies(self, other: Self) -> bool { - !matches!( - (self, other), - (Reliability::Reliable, Reliability::BestEffort) - ) - } - #[cfg(feature = "test")] pub fn rand() -> Self { use rand::Rng; diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index 7e0de9cf6a..3625645943 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -314,11 +314,11 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { let reliability = match (self_reliability, other_reliability) { (None, reliability) | (reliability, None) => reliability, (Some(self_reliability), Some(other_reliability)) => { - if other_reliability.implies(self_reliability) { + if self_reliability == other_reliability { Some(self_reliability) } else { return Err(zerror!( - "The Reliability received in InitAck cannot be substituted with my Reliability" + "The Reliability received in InitAck doesn't match my Reliability" ) .into()); } @@ -398,11 +398,11 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { let reliability = match (self_reliability, other_reliability) { (None, reliability) | (reliability, None) => reliability, (Some(self_reliability), Some(other_reliability)) => { - if self_reliability.implies(other_reliability) { + if self_reliability == other_reliability { Some(other_reliability) } else { return Err(zerror!( - "The Reliability received in InitSyn cannot be substituted for my Reliability" + "The Reliability received in InitSyn doesn't match my Reliability" ) .into()); } From c5078bd5988ef4361519798e8381abc055468b90 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Thu, 5 Sep 2024 12:47:07 +0000 Subject: [PATCH 17/37] Refactor negotiation tests --- .../src/unicast/establishment/ext/qos.rs | 387 ++++++++---------- 1 file changed, 167 insertions(+), 220 deletions(-) diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index 3625645943..ecd18536d4 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -465,269 +465,216 @@ mod tests { Ok(()) } - #[tokio::test] - async fn test_priority_range_negotiation_scenario_1() { - let qos_open = &mut QoS::Enabled { - priorities: None, - reliability: None, - }; - let qos_accept = &mut QoS::Enabled { - priorities: None, - reliability: None, - }; - - match test_negotiation(qos_open, qos_accept).await { + async fn test_negotiation_ok(mut open_qos: QoS, mut accept_qos: QoS, expected_qos: QoS) { + match test_negotiation(&mut open_qos, &mut accept_qos).await { Err(err) => panic!("expected `Ok(())`, got: {err}"), Ok(()) => { - assert_eq!(*qos_open, *qos_accept); - assert_eq!( - *qos_open, - QoS::Enabled { - priorities: None, - reliability: None - } - ); + assert_eq!(open_qos, accept_qos); + assert_eq!(open_qos, expected_qos); } }; } + async fn test_negotiation_err(mut open_qos: QoS, mut accept_qos: QoS) { + if let Ok(()) = test_negotiation(&mut open_qos, &mut accept_qos).await { + panic!("expected `Err(_)`, got `Ok(())`") + } + } + #[tokio::test] - async fn test_priority_range_negotiation_scenario_2() { - let qos_open = &mut QoS::Enabled { - priorities: None, - reliability: None, - }; - let qos_accept = &mut QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), - reliability: None, - }; + async fn test_priority_range_negotiation_scenario_1() { + test_negotiation_ok( + QoS::Enabled { + priorities: None, + reliability: None, + }, + QoS::Enabled { + priorities: None, + reliability: None, + }, + QoS::Enabled { + priorities: None, + reliability: None, + }, + ) + .await + } - match test_negotiation(qos_open, qos_accept).await { - Err(err) => panic!("expected `Ok(())`, got: {err}"), - Ok(()) => { - assert_eq!(*qos_open, *qos_accept); - assert_eq!( - *qos_open, - QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), - reliability: None - } - ); - } - }; + #[tokio::test] + async fn test_priority_range_negotiation_scenario_2() { + test_negotiation_ok( + QoS::Enabled { + priorities: None, + reliability: None, + }, + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }, + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }, + ) + .await } #[tokio::test] async fn test_priority_range_negotiation_scenario_3() { - let qos_open = &mut QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), - reliability: None, - }; - let qos_accept = &mut QoS::Enabled { - priorities: None, - reliability: None, - }; - - match test_negotiation(qos_open, qos_accept).await { - Err(err) => panic!("expected `Ok(())`, got: {err}"), - Ok(()) => { - assert_eq!(*qos_open, *qos_accept); - assert_eq!( - *qos_open, - QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), - reliability: None - } - ); - } - }; + test_negotiation_ok( + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }, + QoS::Enabled { + priorities: None, + reliability: None, + }, + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }, + ) + .await } #[tokio::test] async fn test_priority_range_negotiation_scenario_4() { - let qos_open = &mut QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), - reliability: None, - }; - let qos_accept = &mut QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), - reliability: None, - }; - - match test_negotiation(qos_open, qos_accept).await { - Err(err) => panic!("expected `Ok(())`, got: {err}"), - Ok(()) => { - assert_eq!(*qos_open, *qos_accept); - assert_eq!( - *qos_open, - QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), - reliability: None - } - ); - } - }; + test_negotiation_ok( + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }, + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }, + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }, + ) + .await } #[tokio::test] async fn test_priority_range_negotiation_scenario_5() { - let qos_open = &mut QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), - reliability: None, - }; - let qos_accept = &mut QoS::Enabled { - priorities: Some(PriorityRange::new(0, 4).unwrap()), - reliability: None, - }; - - match test_negotiation(qos_open, qos_accept).await { - Err(err) => panic!("expected `Ok(())`, got: {err}"), - Ok(()) => { - assert_eq!(*qos_open, *qos_accept); - assert_eq!( - *qos_open, - QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), - reliability: None - } - ); - } - }; + test_negotiation_ok( + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }, + QoS::Enabled { + priorities: Some(PriorityRange::new(0, 4).unwrap()), + reliability: None, + }, + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }, + ) + .await } #[tokio::test] async fn test_priority_range_negotiation_scenario_6() { - let qos_open = &mut QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), - reliability: None, - }; - let qos_accept = &mut QoS::Enabled { - priorities: Some(PriorityRange::new(2, 3).unwrap()), - reliability: None, - }; - - if let Ok(()) = test_negotiation(qos_open, qos_accept).await { - panic!("expected `Err(_)`, got `Ok(())`") - } + test_negotiation_err( + QoS::Enabled { + priorities: Some(PriorityRange::new(1, 3).unwrap()), + reliability: None, + }, + QoS::Enabled { + priorities: Some(PriorityRange::new(2, 3).unwrap()), + reliability: None, + }, + ) + .await } #[tokio::test] async fn test_reliability_negotiation_scenario_2() { - let qos_open = &mut QoS::Enabled { - reliability: None, - priorities: None, - }; - let qos_accept = &mut QoS::Enabled { - reliability: Some(Reliability::BestEffort), - priorities: None, - }; - - match test_negotiation(qos_open, qos_accept).await { - Err(err) => panic!("expected `Ok(())`, got: {err}"), - Ok(()) => { - assert_eq!(*qos_open, *qos_accept); - assert_eq!( - *qos_open, - QoS::Enabled { - reliability: Some(Reliability::BestEffort), - priorities: None - } - ); - } - }; + test_negotiation_ok( + QoS::Enabled { + reliability: None, + priorities: None, + }, + QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: None, + }, + QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: None, + }, + ) + .await } #[tokio::test] async fn test_reliability_negotiation_scenario_3() { - let qos_open = &mut QoS::Enabled { - reliability: Some(Reliability::Reliable), - priorities: None, - }; - let qos_accept = &mut QoS::Enabled { - reliability: Some(Reliability::BestEffort), - priorities: None, - }; - - match test_negotiation(qos_open, qos_accept).await { - Err(err) => panic!("expected `Ok(())`, got: {err}"), - Ok(()) => { - assert_eq!(*qos_open, *qos_accept); - assert_eq!( - *qos_open, - QoS::Enabled { - reliability: Some(Reliability::Reliable), - priorities: None - } - ); - } - }; + test_negotiation_err( + QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None, + }, + QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: None, + }, + ) + .await } #[tokio::test] async fn test_reliability_negotiation_scenario_4() { - let qos_open = &mut QoS::Enabled { - reliability: Some(Reliability::Reliable), - priorities: None, - }; - let qos_accept = &mut QoS::Enabled { - reliability: Some(Reliability::Reliable), - priorities: None, - }; - - match test_negotiation(qos_open, qos_accept).await { - Err(err) => panic!("expected `Ok(())`, got: {err}"), - Ok(()) => { - assert_eq!(*qos_open, *qos_accept); - assert_eq!( - *qos_open, - QoS::Enabled { - reliability: Some(Reliability::Reliable), - priorities: None - } - ); - } - }; + test_negotiation_ok( + QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None, + }, + QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None, + }, + QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None, + }, + ) + .await } #[tokio::test] async fn test_reliability_negotiation_scenario_5() { - let qos_open = &mut QoS::Enabled { - reliability: Some(Reliability::BestEffort), - priorities: None, - }; - let qos_accept = &mut QoS::Enabled { - reliability: Some(Reliability::Reliable), - priorities: None, - }; - - if let Ok(()) = test_negotiation(qos_open, qos_accept).await { - panic!("expected `Err(_)`, got `Ok(())`") - } + test_negotiation_err( + QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: None, + }, + QoS::Enabled { + reliability: Some(Reliability::Reliable), + priorities: None, + }, + ) + .await } #[tokio::test] async fn test_priority_range_and_reliability_negotiation_scenario_1() { - let qos_open = &mut QoS::Enabled { - reliability: Some(Reliability::Reliable), - priorities: Some(PriorityRange::new(1, 3).unwrap()), - }; - let qos_accept = &mut QoS::Enabled { - reliability: Some(Reliability::BestEffort), - priorities: Some(PriorityRange::new(1, 4).unwrap()), - }; - - match test_negotiation(qos_open, qos_accept).await { - Err(err) => panic!("expected `Ok(())`, got: {err}"), - Ok(()) => { - assert_eq!(*qos_open, *qos_accept); - assert_eq!( - *qos_open, - QoS::Enabled { - reliability: Some(Reliability::Reliable), - priorities: Some(PriorityRange::new(1, 3).unwrap()) - } - ); - } - }; + test_negotiation_ok( + QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: Some(PriorityRange::new(1, 3).unwrap()), + }, + QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: Some(PriorityRange::new(1, 4).unwrap()), + }, + QoS::Enabled { + reliability: Some(Reliability::BestEffort), + priorities: Some(PriorityRange::new(1, 3).unwrap()), + }, + ) + .await } } From d37499208fd6350fefe7d78ee43975a6b8187af6 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Thu, 5 Sep 2024 14:27:26 +0000 Subject: [PATCH 18/37] We are still not `core::error::Error` --- commons/zenoh-protocol/src/core/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index d7ba72e850..46df149ab5 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -23,7 +23,6 @@ use core::{ hash::Hash, str::FromStr, }; -use std::error::Error; use serde::Serialize; pub use uhlc::{Timestamp, NTP64}; @@ -456,7 +455,8 @@ impl Display for InvalidReliability { } } -impl Error for InvalidReliability {} +#[cfg(feature = "std")] +impl std::error::Error for InvalidReliability {} impl FromStr for Reliability { type Err = InvalidReliability; From ab64d0ff30f134d0167e4e822faef1b2cc00921d Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Thu, 5 Sep 2024 15:15:03 +0000 Subject: [PATCH 19/37] Use `RangeInclusive` --- commons/zenoh-protocol/src/core/mod.rs | 112 +++++++++++++----- .../src/unicast/establishment/ext/qos.rs | 88 ++++++-------- io/zenoh-transport/src/unicast/link.rs | 10 +- .../src/unicast/universal/link.rs | 8 +- .../src/unicast/universal/tx.rs | 47 +++----- 5 files changed, 150 insertions(+), 115 deletions(-) diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index 46df149ab5..31e141d7b7 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -21,13 +21,14 @@ use core::{ convert::{From, TryFrom, TryInto}, fmt::{self, Display}, hash::Hash, + ops::{Deref, RangeInclusive}, str::FromStr, }; use serde::Serialize; pub use uhlc::{Timestamp, NTP64}; use zenoh_keyexpr::OwnedKeyExpr; -use zenoh_result::{bail, zerror, ZResult}; +use zenoh_result::{bail, zerror}; /// The unique Id of the [`HLC`](uhlc::HLC) that generated the concerned [`Timestamp`]. pub type TimestampId = uhlc::ID; @@ -296,7 +297,7 @@ impl EntityGlobalIdProto { } #[repr(u8)] -#[derive(Debug, Default, Copy, Clone, Eq, Hash, PartialEq)] +#[derive(Debug, Default, Copy, Clone, Eq, Hash, PartialEq, PartialOrd, Ord, Serialize)] pub enum Priority { Control = 0, RealTime = 1, @@ -309,46 +310,31 @@ pub enum Priority { Background = 7, } -#[derive(Debug, Default, Copy, Clone, Eq, Hash, PartialEq, Serialize)] +// TODO: Use Priority type +#[derive(Debug, Clone, Eq, Hash, PartialEq, Serialize)] /// A `u8` range bounded inclusively below and above. -pub struct PriorityRange { - pub start: u8, - pub end: u8, -} +pub struct PriorityRange(RangeInclusive); -impl PriorityRange { - pub fn new(start: u8, end: u8) -> ZResult { - if start > end || start < Priority::MAX as u8 || end > Priority::MIN as u8 { - bail!("Invalid priority range: {start}..{end}") - }; +impl Deref for PriorityRange { + type Target = RangeInclusive; - Ok(Self { start, end }) + fn deref(&self) -> &Self::Target { + &self.0 } +} - /// Returns `true` if `priority` is a member of `self`. - pub fn contains(&self, priority: Priority) -> bool { - self.start <= (priority as u8) && (priority as u8) <= self.end +impl PriorityRange { + pub fn new(range: RangeInclusive) -> Self { + Self(range) } /// Returns `true` if `self` is a superset of `other`. - pub fn includes(&self, other: PriorityRange) -> bool { - self.start <= other.start && other.end <= self.end + pub fn includes(&self, other: &PriorityRange) -> bool { + self.start() <= other.start() && other.end() <= self.end() } pub fn len(&self) -> usize { - (self.end - self.start + 1) as usize - } - - pub fn is_empty(&self) -> bool { - self.end == self.start - } - - pub fn start(&self) -> u8 { - self.start - } - - pub fn end(&self) -> u8 { - self.end + *self.end() as usize - *self.start() as usize + 1 // 1..=3, 3-1 == 2 } #[cfg(feature = "test")] @@ -358,7 +344,69 @@ impl PriorityRange { let start = rng.gen_range(Priority::MAX as u8..Priority::MIN as u8); let end = rng.gen_range((start + 1)..=Priority::MIN as u8); - Self { start, end } + Self(Priority::try_from(start).unwrap()..=Priority::try_from(end).unwrap()) + } +} + +#[derive(Debug)] +pub enum InvalidPriorityRange { + InvalidSyntax { found: String }, + InvalidBound { message: String }, +} + +impl Display for InvalidPriorityRange { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + InvalidPriorityRange::InvalidSyntax { found } => write!(f, "invalid PriorityRange string, expected an range of the form `start..=end` but found {found}"), + InvalidPriorityRange::InvalidBound { message } => write!(f, "invalid PriorityRange bound: {message}"), + } + } +} + +#[cfg(feature = "std")] +impl std::error::Error for InvalidPriorityRange {} + +impl FromStr for PriorityRange { + type Err = InvalidPriorityRange; + + fn from_str(s: &str) -> Result { + let mut metadata = s.split("..="); + + let start = metadata + .next() + .ok_or_else(|| InvalidPriorityRange::InvalidSyntax { + found: s.to_string(), + })? + .parse::() + .map(Priority::try_from) + .map_err(|err| InvalidPriorityRange::InvalidBound { + message: err.to_string(), + })? + .map_err(|err| InvalidPriorityRange::InvalidBound { + message: err.to_string(), + })?; + + let end = metadata + .next() + .ok_or_else(|| InvalidPriorityRange::InvalidSyntax { + found: s.to_string(), + })? + .parse::() + .map(Priority::try_from) + .map_err(|err| InvalidPriorityRange::InvalidBound { + message: err.to_string(), + })? + .map_err(|err| InvalidPriorityRange::InvalidBound { + message: err.to_string(), + })?; + + if metadata.next().is_some() { + return Err(InvalidPriorityRange::InvalidSyntax { + found: s.to_string(), + }); + }; + + Ok(PriorityRange::new(start..=end)) } } diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index ecd18536d4..101835c1b7 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -21,10 +21,10 @@ use zenoh_buffers::{ writer::{DidntWrite, Writer}, }; use zenoh_codec::{RCodec, WCodec, Zenoh080}; -use zenoh_core::{bail, zerror}; +use zenoh_core::zerror; use zenoh_link::EndPoint; use zenoh_protocol::{ - core::{PriorityRange, Reliability}, + core::{Priority, PriorityRange, Reliability}, transport::{init, open}, }; use zenoh_result::{Error as ZError, ZResult}; @@ -42,7 +42,7 @@ impl<'a> QoSFsm<'a> { } } -#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize)] +#[derive(Clone, Debug, PartialEq, Eq, Serialize)] pub(crate) enum QoS { Disabled, Enabled { @@ -68,25 +68,7 @@ impl QoS { let priorities = metadata .get(PRIORITY_METADATA_KEY) - .map(|metadata| { - let mut metadata = metadata.split(".."); - - let start = metadata - .next() - .ok_or(zerror!("Invalid priority range syntax"))? - .parse::()?; - - let end = metadata - .next() - .ok_or(zerror!("Invalid priority range syntax"))? - .parse::()?; - - if metadata.next().is_some() { - bail!("Invalid priority range syntax") - }; - - PriorityRange::new(start, end) - }) + .map(PriorityRange::from_str) .transpose()?; Ok(QoS::Enabled { @@ -107,10 +89,10 @@ impl QoS { let tag = value & 0b111_u64; let priorities = if tag & 0b010_u64 != 0 { - let start = ((value >> 3) & 0xff) as u8; - let end = ((value >> (3 + 8)) & 0xff) as u8; + let start = Priority::try_from(((value >> 3) & 0xff) as u8)?; + let end = Priority::try_from(((value >> (3 + 8)) & 0xff) as u8)?; - Some(PriorityRange::new(start, end)?) + Some(PriorityRange::new(start..=end)) } else { None }; @@ -141,7 +123,7 @@ impl QoS { /// 3. QoS is enabled and priority range is available but reliability is unavailable /// 4. QoS is enabled and reliability is available but priority range is unavailable /// 5. QoS is enabled and both priority range and reliability are available - fn to_u64(self) -> u64 { + fn to_u64(&self) -> u64 { match self { QoS::Disabled => 0b000_u64, QoS::Enabled { @@ -156,13 +138,13 @@ impl QoS { if let Some(priorities) = priorities { value |= 0b010_u64; - value |= (priorities.start() as u64) << 3; - value |= (priorities.end() as u64) << (3 + 8); + value |= (*priorities.start() as u64) << 3; + value |= (*priorities.end() as u64) << (3 + 8); } if let Some(reliability) = reliability { value |= 0b100_u64; - value |= (bool::from(reliability) as u64) << (3 + 8 + 8); + value |= (bool::from(*reliability) as u64) << (3 + 8 + 8); } value @@ -170,7 +152,7 @@ impl QoS { } } - fn to_ext(self) -> Option { + fn to_ext(&self) -> Option { if self.is_enabled() { Some(init::ext::QoS::new(self.to_u64())) } else { @@ -199,7 +181,7 @@ impl QoS { QoS::Enabled { priorities: Some(priorities), .. - } => Some(*priorities), + } => Some(priorities.clone()), } } @@ -291,7 +273,7 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { reliability: other_reliability, priorities: other_priorities, }, - ) = (*state_self, state_other) + ) = (state_self.clone(), state_other) else { *state_self = QoS::Disabled; return Ok(()); @@ -300,7 +282,7 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { let priorities = match (self_priorities, other_priorities) { (None, priorities) | (priorities, None) => priorities, (Some(self_priorities), Some(other_priorities)) => { - if other_priorities.includes(self_priorities) { + if other_priorities.includes(&self_priorities) { Some(self_priorities) } else { return Err(zerror!( @@ -375,7 +357,7 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { reliability: other_reliability, priorities: other_priorities, }, - ) = (*state_self, state_other) + ) = (state_self.clone(), state_other) else { *state_self = QoS::Disabled; return Ok(()); @@ -384,7 +366,7 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { let priorities = match (self_priorities, other_priorities) { (None, priorities) | (priorities, None) => priorities, (Some(self_priorities), Some(other_priorities)) => { - if self_priorities.includes(other_priorities) { + if self_priorities.includes(&other_priorities) { Some(other_priorities) } else { return Err(zerror!( @@ -453,6 +435,12 @@ mod tests { use super::{QoS, QoSFsm}; use crate::unicast::establishment::{AcceptFsm, OpenFsm}; + macro_rules! priority_range { + ($start:literal, $end:literal) => { + PriorityRange::new($start.try_into().unwrap()..=$end.try_into().unwrap()) + }; + } + async fn test_negotiation(qos_open: &mut QoS, qos_accept: &mut QoS) -> ZResult<()> { let fsm = QoSFsm::new(); @@ -508,11 +496,11 @@ mod tests { reliability: None, }, QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, ) @@ -523,7 +511,7 @@ mod tests { async fn test_priority_range_negotiation_scenario_3() { test_negotiation_ok( QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, QoS::Enabled { @@ -531,7 +519,7 @@ mod tests { reliability: None, }, QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, ) @@ -542,15 +530,15 @@ mod tests { async fn test_priority_range_negotiation_scenario_4() { test_negotiation_ok( QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, ) @@ -561,15 +549,15 @@ mod tests { async fn test_priority_range_negotiation_scenario_5() { test_negotiation_ok( QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, QoS::Enabled { - priorities: Some(PriorityRange::new(0, 4).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, ) @@ -580,11 +568,11 @@ mod tests { async fn test_priority_range_negotiation_scenario_6() { test_negotiation_err( QoS::Enabled { - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, QoS::Enabled { - priorities: Some(PriorityRange::new(2, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), reliability: None, }, ) @@ -664,15 +652,15 @@ mod tests { test_negotiation_ok( QoS::Enabled { reliability: Some(Reliability::BestEffort), - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), }, QoS::Enabled { reliability: Some(Reliability::BestEffort), - priorities: Some(PriorityRange::new(1, 4).unwrap()), + priorities: Some(priority_range!(1, 3)), }, QoS::Enabled { reliability: Some(Reliability::BestEffort), - priorities: Some(PriorityRange::new(1, 3).unwrap()), + priorities: Some(priority_range!(1, 3)), }, ) .await diff --git a/io/zenoh-transport/src/unicast/link.rs b/io/zenoh-transport/src/unicast/link.rs index bfd69ff7dc..73862821df 100644 --- a/io/zenoh-transport/src/unicast/link.rs +++ b/io/zenoh-transport/src/unicast/link.rs @@ -30,7 +30,7 @@ pub(crate) enum TransportLinkUnicastDirection { Outbound, } -#[derive(Clone, Copy, PartialEq, Eq, Debug)] +#[derive(Clone, PartialEq, Eq, Debug)] pub(crate) struct TransportLinkUnicastConfig { // Inbound / outbound pub(crate) direction: TransportLinkUnicastDirection, @@ -60,7 +60,11 @@ impl TransportLinkUnicast { } pub(crate) fn link(&self) -> Link { - Link::new_unicast(&self.link, self.config.priorities, self.config.reliability) + Link::new_unicast( + &self.link, + self.config.priorities.clone(), + self.config.reliability, + ) } pub(crate) fn tx(&self) -> TransportLinkUnicastTx { @@ -82,7 +86,7 @@ impl TransportLinkUnicast { pub(crate) fn rx(&self) -> TransportLinkUnicastRx { TransportLinkUnicastRx { link: self.link.clone(), - config: self.config, + config: self.config.clone(), } } diff --git a/io/zenoh-transport/src/unicast/universal/link.rs b/io/zenoh-transport/src/unicast/universal/link.rs index 960b2e1f34..bb88a912d4 100644 --- a/io/zenoh-transport/src/unicast/universal/link.rs +++ b/io/zenoh-transport/src/unicast/universal/link.rs @@ -114,7 +114,7 @@ impl TransportLinkUnicastUniversal { } pub(super) fn start_rx(&mut self, transport: TransportUnicastUniversal, lease: Duration) { - let priorities = self.link.config.priorities; + let priorities = self.link.config.priorities.clone(); let reliability = self.link.config.reliability; let mut rx = self.link.rx(); let token = self.token.clone(); @@ -262,7 +262,11 @@ async fn rx_task( } let pool = RecyclingObjectPool::new(n, || vec![0_u8; mtu].into_boxed_slice()); - let l = Link::new_unicast(&link.link, link.config.priorities, link.config.reliability); + let l = Link::new_unicast( + &link.link, + link.config.priorities.clone(), + link.config.reliability, + ); loop { tokio::select! { diff --git a/io/zenoh-transport/src/unicast/universal/tx.rs b/io/zenoh-transport/src/unicast/universal/tx.rs index ccbf9fecba..27e721227c 100644 --- a/io/zenoh-transport/src/unicast/universal/tx.rs +++ b/io/zenoh-transport/src/unicast/universal/tx.rs @@ -46,7 +46,7 @@ impl TransportUnicastUniversal { let (match_, _) = elements.enumerate().fold( (Match::default(), Option::::None), |(mut match_, mut prev_priorities), (i, (r, ps))| { - match (r.eq(&reliability), ps.filter(|ps| ps.contains(priority))) { + match (r.eq(&reliability), ps.filter(|ps| ps.contains(&priority))) { (true, Some(priorities)) if prev_priorities .as_ref() @@ -74,9 +74,12 @@ impl TransportUnicastUniversal { .expect("reading `TransportUnicastUniversal::links` should not fail"); let Some(transport_link_index) = Self::select( - transport_links - .iter() - .map(|tl| (tl.link.config.reliability, tl.link.config.priorities)), + transport_links.iter().map(|tl| { + ( + tl.link.config.reliability, + tl.link.config.priorities.clone(), + ) + }), Reliability::from(msg.is_reliable()), msg.priority(), ) else { @@ -138,23 +141,20 @@ mod tests { use crate::unicast::universal::transport::TransportUnicastUniversal; + macro_rules! priority_range { + ($start:literal, $end:literal) => { + PriorityRange::new($start.try_into().unwrap()..=$end.try_into().unwrap()) + }; + } + #[test] /// Tests the "full match" scenario with exactly one candidate. fn test_link_selection_scenario_1() { let selection = TransportUnicastUniversal::select( [ - ( - Reliability::Reliable, - Some(PriorityRange::new(0, 1).unwrap()), - ), - ( - Reliability::Reliable, - Some(PriorityRange::new(1, 2).unwrap()), - ), - ( - Reliability::BestEffort, - Some(PriorityRange::new(0, 1).unwrap()), - ), + (Reliability::Reliable, Some(priority_range!(0, 1))), + (Reliability::Reliable, Some(priority_range!(1, 2))), + (Reliability::BestEffort, Some(priority_range!(0, 1))), ] .into_iter(), Reliability::Reliable, @@ -168,14 +168,8 @@ mod tests { fn test_link_selection_scenario_2() { let selection = TransportUnicastUniversal::select( [ - ( - Reliability::Reliable, - Some(PriorityRange::new(0, 2).unwrap()), - ), - ( - Reliability::Reliable, - Some(PriorityRange::new(0, 1).unwrap()), - ), + (Reliability::Reliable, Some(priority_range!(0, 2))), + (Reliability::Reliable, Some(priority_range!(0, 1))), ] .into_iter(), Reliability::Reliable, @@ -189,10 +183,7 @@ mod tests { fn test_link_selection_scenario_3() { let selection = TransportUnicastUniversal::select( [ - ( - Reliability::BestEffort, - Some(PriorityRange::new(0, 1).unwrap()), - ), + (Reliability::BestEffort, Some(priority_range!(0, 1))), (Reliability::Reliable, None), ] .into_iter(), From 2f9edc14b537be588948ba42cf1f5ea42a039489 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Thu, 5 Sep 2024 15:17:51 +0000 Subject: [PATCH 20/37] Clippy at it again --- commons/zenoh-protocol/src/core/mod.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index 31e141d7b7..c7f674f821 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -310,7 +310,6 @@ pub enum Priority { Background = 7, } -// TODO: Use Priority type #[derive(Debug, Clone, Eq, Hash, PartialEq, Serialize)] /// A `u8` range bounded inclusively below and above. pub struct PriorityRange(RangeInclusive); @@ -334,7 +333,11 @@ impl PriorityRange { } pub fn len(&self) -> usize { - *self.end() as usize - *self.start() as usize + 1 // 1..=3, 3-1 == 2 + *self.end() as usize - *self.start() as usize + 1 + } + + pub fn is_empty(&self) -> bool { + false } #[cfg(feature = "test")] From a941021012bd82daa5dd39644670a884c9ddbe66 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Fri, 6 Sep 2024 07:23:41 +0000 Subject: [PATCH 21/37] Split `State` into `StateOpen` and `StateAccept` --- commons/zenoh-protocol/src/core/mod.rs | 2 +- io/zenoh-link-commons/src/lib.rs | 1 + io/zenoh-link-commons/src/unicast.rs | 1 + .../src/unicast/establishment/accept.rs | 6 +- .../src/unicast/establishment/cookie.rs | 6 +- .../src/unicast/establishment/ext/qos.rs | 298 +++++++++++------- .../src/unicast/establishment/open.rs | 6 +- 7 files changed, 199 insertions(+), 121 deletions(-) diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index c7f674f821..02b1cf0c95 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -311,7 +311,7 @@ pub enum Priority { } #[derive(Debug, Clone, Eq, Hash, PartialEq, Serialize)] -/// A `u8` range bounded inclusively below and above. +/// A [`Priority`] range bounded inclusively below and above. pub struct PriorityRange(RangeInclusive); impl Deref for PriorityRange { diff --git a/io/zenoh-link-commons/src/lib.rs b/io/zenoh-link-commons/src/lib.rs index c3fe1aee41..dd26789ff2 100644 --- a/io/zenoh-link-commons/src/lib.rs +++ b/io/zenoh-link-commons/src/lib.rs @@ -45,6 +45,7 @@ use zenoh_result::ZResult; pub const BIND_INTERFACE: &str = "iface"; +// TODO(fuzzypixelz): Patch the Locators to contain negotiated priority and reliability #[derive(Clone, Debug, Serialize, Hash, PartialEq, Eq)] pub struct Link { pub src: Locator, diff --git a/io/zenoh-link-commons/src/unicast.rs b/io/zenoh-link-commons/src/unicast.rs index c29f4c7c14..15d7ce8b17 100644 --- a/io/zenoh-link-commons/src/unicast.rs +++ b/io/zenoh-link-commons/src/unicast.rs @@ -38,6 +38,7 @@ pub trait LinkManagerUnicastTrait: Send + Sync { } pub type NewLinkChannelSender = flume::Sender; +// TODO(fuzzypixelz): Delete this /// Notification of a new inbound connection. /// /// Link implementations should preserve the metadata sections of [`NewLinkUnicast::endpoint`]. diff --git a/io/zenoh-transport/src/unicast/establishment/accept.rs b/io/zenoh-transport/src/unicast/establishment/accept.rs index 94baee52fe..ef95ee7557 100644 --- a/io/zenoh-transport/src/unicast/establishment/accept.rs +++ b/io/zenoh-transport/src/unicast/establishment/accept.rs @@ -55,7 +55,7 @@ pub(super) type AcceptError = (zenoh_result::Error, Option); struct StateTransport { batch_size: BatchSize, resolution: Resolution, - ext_qos: ext::qos::QoS, + ext_qos: ext::qos::StateAccept, #[cfg(feature = "transport_multilink")] ext_mlink: ext::multilink::StateAccept, #[cfg(feature = "shared-memory")] @@ -706,7 +706,7 @@ pub(crate) async fn accept_link( transport: StateTransport { batch_size, resolution: manager.config.resolution, - ext_qos: ext::qos::QoS::new(manager.config.unicast.is_qos, &endpoint)?, + ext_qos: ext::qos::StateAccept::new(manager.config.unicast.is_qos, &endpoint)?, #[cfg(feature = "transport_multilink")] ext_mlink: manager .state @@ -774,7 +774,7 @@ pub(crate) async fn accept_link( whatami: osyn_out.other_whatami, sn_resolution: state.transport.resolution.get(Field::FrameSN), tx_initial_sn: oack_out.open_ack.initial_sn, - is_qos: state.transport.ext_qos.is_enabled(), + is_qos: state.transport.ext_qos.is_qos(), #[cfg(feature = "transport_multilink")] multilink: state.transport.ext_mlink.multilink(), #[cfg(feature = "shared-memory")] diff --git a/io/zenoh-transport/src/unicast/establishment/cookie.rs b/io/zenoh-transport/src/unicast/establishment/cookie.rs index 7fb84f258e..4220f8e08b 100644 --- a/io/zenoh-transport/src/unicast/establishment/cookie.rs +++ b/io/zenoh-transport/src/unicast/establishment/cookie.rs @@ -34,7 +34,7 @@ pub(crate) struct Cookie { pub(crate) batch_size: BatchSize, pub(crate) nonce: u64, // Extensions - pub(crate) ext_qos: ext::qos::QoS, + pub(crate) ext_qos: ext::qos::StateAccept, #[cfg(feature = "transport_multilink")] pub(crate) ext_mlink: ext::multilink::StateAccept, #[cfg(feature = "shared-memory")] @@ -90,7 +90,7 @@ where let batch_size: BatchSize = self.read(&mut *reader)?; let nonce: u64 = self.read(&mut *reader)?; // Extensions - let ext_qos: ext::qos::QoS = self.read(&mut *reader)?; + let ext_qos: ext::qos::StateAccept = self.read(&mut *reader)?; #[cfg(feature = "transport_multilink")] let ext_mlink: ext::multilink::StateAccept = self.read(&mut *reader)?; #[cfg(feature = "shared-memory")] @@ -178,7 +178,7 @@ impl Cookie { resolution: Resolution::rand(), batch_size: rng.gen(), nonce: rng.gen(), - ext_qos: ext::qos::QoS::rand(), + ext_qos: ext::qos::StateAccept::rand(), #[cfg(feature = "transport_multilink")] ext_mlink: ext::multilink::StateAccept::rand(), #[cfg(feature = "shared-memory")] diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index 101835c1b7..36ab15b077 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -15,7 +15,6 @@ use core::marker::PhantomData; use std::str::FromStr; use async_trait::async_trait; -use serde::Serialize; use zenoh_buffers::{ reader::{DidntRead, Reader}, writer::{DidntWrite, Writer}, @@ -42,19 +41,20 @@ impl<'a> QoSFsm<'a> { } } -#[derive(Clone, Debug, PartialEq, Eq, Serialize)] -pub(crate) enum QoS { - Disabled, - Enabled { +// TODO(fuzzypixelz): Fallback to ZExtUnit QoS matching QoS::Disabled or QoS::Enabled { reliability: None, priorities: None } +#[derive(Clone, Debug, PartialEq, Eq)] +enum State { + NoQoS, + QoS { reliability: Option, priorities: Option, }, } -impl QoS { - pub(crate) fn new(is_qos: bool, endpoint: &EndPoint) -> ZResult { +impl State { + fn new(is_qos: bool, endpoint: &EndPoint) -> ZResult { if !is_qos { - Ok(QoS::Disabled) + Ok(State::NoQoS) } else { const RELIABILITY_METADATA_KEY: &str = "reliability"; const PRIORITY_METADATA_KEY: &str = "priorities"; @@ -71,7 +71,7 @@ impl QoS { .map(PriorityRange::from_str) .transpose()?; - Ok(QoS::Enabled { + Ok(State::QoS { priorities, reliability, }) @@ -80,8 +80,8 @@ impl QoS { fn try_from_u64(value: u64) -> ZResult { match value { - 0b000_u64 => Ok(QoS::Disabled), - 0b001_u64 => Ok(QoS::Enabled { + 0b000_u64 => Ok(State::NoQoS), + 0b001_u64 => Ok(State::QoS { priorities: None, reliability: None, }), @@ -105,7 +105,7 @@ impl QoS { None }; - Ok(QoS::Enabled { + Ok(State::QoS { priorities, reliability, }) @@ -116,6 +116,10 @@ impl QoS { /// Encodes [`QoS`] as a [`u64`]. /// + /// This function is used for encoding both of [`StateAccept`] in + /// [`crate::unicast::establishment::cookie::Cookie::ext_qos`] and + /// [`zenoh_protocol::transport::init::ext::QoS`]. + /// /// The three least significant bits are used to discrimnate five states: /// /// 1. QoS is disabled @@ -125,8 +129,8 @@ impl QoS { /// 5. QoS is enabled and both priority range and reliability are available fn to_u64(&self) -> u64 { match self { - QoS::Disabled => 0b000_u64, - QoS::Enabled { + State::NoQoS => 0b000_u64, + State::QoS { priorities, reliability, } => { @@ -153,7 +157,7 @@ impl QoS { } fn to_ext(&self) -> Option { - if self.is_enabled() { + if self.is_qos() { Some(init::ext::QoS::new(self.to_u64())) } else { None @@ -162,36 +166,36 @@ impl QoS { fn try_from_ext(ext: Option) -> ZResult { if let Some(ext) = ext { - QoS::try_from_u64(ext.value) + State::try_from_u64(ext.value) } else { - Ok(QoS::Disabled) + Ok(State::NoQoS) } } - pub(crate) fn is_enabled(&self) -> bool { - matches!(self, QoS::Enabled { .. }) + fn is_qos(&self) -> bool { + matches!(self, State::QoS { .. }) } - pub(crate) fn priorities(&self) -> Option { + fn priorities(&self) -> Option { match self { - QoS::Disabled - | QoS::Enabled { + State::NoQoS + | State::QoS { priorities: None, .. } => None, - QoS::Enabled { + State::QoS { priorities: Some(priorities), .. } => Some(priorities.clone()), } } - pub(crate) fn reliability(&self) -> Option { + fn reliability(&self) -> Option { match self { - QoS::Disabled - | QoS::Enabled { + State::NoQoS + | State::QoS { reliability: None, .. } => None, - QoS::Enabled { + State::QoS { reliability: Some(reliability), .. } => Some(*reliability), @@ -199,18 +203,18 @@ impl QoS { } #[cfg(test)] - pub(crate) fn rand() -> Self { + fn rand() -> Self { use rand::Rng; let mut rng = rand::thread_rng(); if rng.gen_bool(0.5) { - QoS::Disabled + State::NoQoS } else { let priorities = rng.gen_bool(0.5).then(PriorityRange::rand); let reliability = rng .gen_bool(0.5) .then(|| Reliability::from(rng.gen_bool(0.5))); - QoS::Enabled { + State::QoS { priorities, reliability, } @@ -218,26 +222,30 @@ impl QoS { } } -// Codec -impl WCodec<&QoS, &mut W> for Zenoh080 -where - W: Writer, -{ - type Output = Result<(), DidntWrite>; +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct StateOpen(State); - fn write(self, writer: &mut W, x: &QoS) -> Self::Output { - self.write(writer, &x.to_u64()) +impl From for StateOpen { + fn from(value: State) -> Self { + StateOpen(value) } } -impl RCodec for Zenoh080 -where - R: Reader, -{ - type Error = DidntRead; +impl StateOpen { + pub(crate) fn new(is_qos: bool, endpoint: &EndPoint) -> ZResult { + State::new(is_qos, endpoint).map(StateOpen) + } - fn read(self, reader: &mut R) -> Result { - QoS::try_from_u64(self.read(reader)?).map_err(|_| DidntRead) + pub(crate) fn is_qos(&self) -> bool { + self.0.is_qos() + } + + pub(crate) fn priorities(&self) -> Option { + self.0.priorities() + } + + pub(crate) fn reliability(&self) -> Option { + self.0.reliability() } } @@ -245,16 +253,16 @@ where impl<'a> OpenFsm for &'a QoSFsm<'a> { type Error = ZError; - type SendInitSynIn = &'a QoS; + type SendInitSynIn = &'a StateOpen; type SendInitSynOut = Option; async fn send_init_syn( self, state: Self::SendInitSynIn, ) -> Result { - Ok(state.to_ext()) + Ok(state.0.to_ext()) } - type RecvInitAckIn = (&'a mut QoS, Option); + type RecvInitAckIn = (&'a mut StateOpen, Option); type RecvInitAckOut = (); async fn recv_init_ack( self, @@ -262,20 +270,20 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { ) -> Result { let (state_self, other_ext) = input; - let state_other = QoS::try_from_ext(other_ext)?; + let state_other = State::try_from_ext(other_ext)?; let ( - QoS::Enabled { + State::QoS { reliability: self_reliability, priorities: self_priorities, }, - QoS::Enabled { + State::QoS { reliability: other_reliability, priorities: other_priorities, }, - ) = (state_self.clone(), state_other) + ) = (state_self.0.clone(), state_other) else { - *state_self = QoS::Disabled; + *state_self = State::NoQoS.into(); return Ok(()); }; @@ -307,15 +315,16 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { } }; - *state_self = QoS::Enabled { + *state_self = State::QoS { reliability, priorities, - }; + } + .into(); Ok(()) } - type SendOpenSynIn = &'a QoS; + type SendOpenSynIn = &'a StateOpen; type SendOpenSynOut = Option; async fn send_open_syn( self, @@ -324,7 +333,7 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { Ok(None) } - type RecvOpenAckIn = (&'a mut QoS, Option); + type RecvOpenAckIn = (&'a mut StateOpen, Option); type RecvOpenAckOut = (); async fn recv_open_ack( self, @@ -334,11 +343,68 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { } } +#[derive(Clone, Debug, PartialEq, Eq)] +pub(crate) struct StateAccept(State); + +impl From for StateAccept { + fn from(value: State) -> Self { + StateAccept(value) + } +} + +impl StateAccept { + pub(crate) fn new(is_qos: bool, endpoint: &EndPoint) -> ZResult { + State::new(is_qos, endpoint).map(StateAccept::from) + } + + pub(crate) fn is_qos(&self) -> bool { + self.0.is_qos() + } + + pub(crate) fn priorities(&self) -> Option { + self.0.priorities() + } + + pub(crate) fn reliability(&self) -> Option { + self.0.reliability() + } + + #[cfg(test)] + pub(crate) fn rand() -> Self { + State::rand().into() + } +} + +// Codec +impl WCodec<&StateAccept, &mut W> for Zenoh080 +where + W: Writer, +{ + type Output = Result<(), DidntWrite>; + + fn write(self, writer: &mut W, x: &StateAccept) -> Self::Output { + self.write(writer, &x.0.to_u64()) + } +} + +impl RCodec for Zenoh080 +where + R: Reader, +{ + type Error = DidntRead; + + fn read(self, reader: &mut R) -> Result { + Ok(State::try_from_u64(self.read(reader)?) + .map_err(|_| DidntRead)? + .into()) + } +} + #[async_trait] impl<'a> AcceptFsm for &'a QoSFsm<'a> { type Error = ZError; - type RecvInitSynIn = (&'a mut QoS, Option); + type RecvInitSynIn = (&'a mut StateAccept, Option); type RecvInitSynOut = (); async fn recv_init_syn( self, @@ -346,20 +412,20 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { ) -> Result { let (state_self, other_ext) = input; - let state_other = QoS::try_from_ext(other_ext)?; + let state_other = State::try_from_ext(other_ext)?; let ( - QoS::Enabled { + State::QoS { reliability: self_reliability, priorities: self_priorities, }, - QoS::Enabled { + State::QoS { reliability: other_reliability, priorities: other_priorities, }, - ) = (state_self.clone(), state_other) + ) = (state_self.0.clone(), state_other) else { - *state_self = QoS::Disabled; + *state_self = State::NoQoS.into(); return Ok(()); }; @@ -391,24 +457,25 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { } }; - *state_self = QoS::Enabled { + *state_self = State::QoS { reliability, priorities, - }; + } + .into(); Ok(()) } - type SendInitAckIn = &'a QoS; + type SendInitAckIn = &'a StateAccept; type SendInitAckOut = Option; async fn send_init_ack( self, state: Self::SendInitAckIn, ) -> Result { - Ok(state.to_ext()) + Ok(state.0.to_ext()) } - type RecvOpenSynIn = (&'a mut QoS, Option); + type RecvOpenSynIn = (&'a mut StateAccept, Option); type RecvOpenSynOut = (); async fn recv_open_syn( self, @@ -417,7 +484,7 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { Ok(()) } - type SendOpenAckIn = &'a QoS; + type SendOpenAckIn = &'a StateAccept; type SendOpenAckOut = Option; async fn send_open_ack( self, @@ -432,7 +499,7 @@ mod tests { use zenoh_protocol::core::{PriorityRange, Reliability}; use zenoh_result::ZResult; - use super::{QoS, QoSFsm}; + use super::{QoSFsm, State, StateAccept, StateOpen}; use crate::unicast::establishment::{AcceptFsm, OpenFsm}; macro_rules! priority_range { @@ -441,30 +508,39 @@ mod tests { }; } - async fn test_negotiation(qos_open: &mut QoS, qos_accept: &mut QoS) -> ZResult<()> { + async fn test_negotiation( + state_open: &mut StateOpen, + state_accept: &mut StateAccept, + ) -> ZResult<()> { let fsm = QoSFsm::new(); - let ext = fsm.send_init_syn(&*qos_open).await?; - fsm.recv_init_syn((qos_accept, ext)).await?; + let ext = fsm.send_init_syn(&*state_open).await?; + fsm.recv_init_syn((state_accept, ext)).await?; - let ext = fsm.send_init_ack(&*qos_accept).await?; - fsm.recv_init_ack((qos_open, ext)).await?; + let ext = fsm.send_init_ack(&*state_accept).await?; + fsm.recv_init_ack((state_open, ext)).await?; Ok(()) } - async fn test_negotiation_ok(mut open_qos: QoS, mut accept_qos: QoS, expected_qos: QoS) { - match test_negotiation(&mut open_qos, &mut accept_qos).await { + async fn test_negotiation_ok(state_open: State, state_accept: State, state_expected: State) { + let mut state_open = state_open.into(); + let mut state_accept = state_accept.into(); + + match test_negotiation(&mut state_open, &mut state_accept).await { Err(err) => panic!("expected `Ok(())`, got: {err}"), Ok(()) => { - assert_eq!(open_qos, accept_qos); - assert_eq!(open_qos, expected_qos); + assert_eq!(state_open.0, state_accept.0); + assert_eq!(state_open.0, state_expected); } }; } - async fn test_negotiation_err(mut open_qos: QoS, mut accept_qos: QoS) { - if let Ok(()) = test_negotiation(&mut open_qos, &mut accept_qos).await { + async fn test_negotiation_err(state_open: State, state_accept: State) { + let mut state_open = state_open.into(); + let mut state_accept = state_accept.into(); + + if let Ok(()) = test_negotiation(&mut state_open, &mut state_accept).await { panic!("expected `Err(_)`, got `Ok(())`") } } @@ -472,15 +548,15 @@ mod tests { #[tokio::test] async fn test_priority_range_negotiation_scenario_1() { test_negotiation_ok( - QoS::Enabled { + State::QoS { priorities: None, reliability: None, }, - QoS::Enabled { + State::QoS { priorities: None, reliability: None, }, - QoS::Enabled { + State::QoS { priorities: None, reliability: None, }, @@ -491,15 +567,15 @@ mod tests { #[tokio::test] async fn test_priority_range_negotiation_scenario_2() { test_negotiation_ok( - QoS::Enabled { + State::QoS { priorities: None, reliability: None, }, - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, @@ -510,15 +586,15 @@ mod tests { #[tokio::test] async fn test_priority_range_negotiation_scenario_3() { test_negotiation_ok( - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, - QoS::Enabled { + State::QoS { priorities: None, reliability: None, }, - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, @@ -529,15 +605,15 @@ mod tests { #[tokio::test] async fn test_priority_range_negotiation_scenario_4() { test_negotiation_ok( - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, @@ -548,15 +624,15 @@ mod tests { #[tokio::test] async fn test_priority_range_negotiation_scenario_5() { test_negotiation_ok( - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, @@ -567,11 +643,11 @@ mod tests { #[tokio::test] async fn test_priority_range_negotiation_scenario_6() { test_negotiation_err( - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, - QoS::Enabled { + State::QoS { priorities: Some(priority_range!(1, 3)), reliability: None, }, @@ -582,15 +658,15 @@ mod tests { #[tokio::test] async fn test_reliability_negotiation_scenario_2() { test_negotiation_ok( - QoS::Enabled { + State::QoS { reliability: None, priorities: None, }, - QoS::Enabled { + State::QoS { reliability: Some(Reliability::BestEffort), priorities: None, }, - QoS::Enabled { + State::QoS { reliability: Some(Reliability::BestEffort), priorities: None, }, @@ -601,11 +677,11 @@ mod tests { #[tokio::test] async fn test_reliability_negotiation_scenario_3() { test_negotiation_err( - QoS::Enabled { + State::QoS { reliability: Some(Reliability::Reliable), priorities: None, }, - QoS::Enabled { + State::QoS { reliability: Some(Reliability::BestEffort), priorities: None, }, @@ -616,15 +692,15 @@ mod tests { #[tokio::test] async fn test_reliability_negotiation_scenario_4() { test_negotiation_ok( - QoS::Enabled { + State::QoS { reliability: Some(Reliability::Reliable), priorities: None, }, - QoS::Enabled { + State::QoS { reliability: Some(Reliability::Reliable), priorities: None, }, - QoS::Enabled { + State::QoS { reliability: Some(Reliability::Reliable), priorities: None, }, @@ -635,11 +711,11 @@ mod tests { #[tokio::test] async fn test_reliability_negotiation_scenario_5() { test_negotiation_err( - QoS::Enabled { + State::QoS { reliability: Some(Reliability::BestEffort), priorities: None, }, - QoS::Enabled { + State::QoS { reliability: Some(Reliability::Reliable), priorities: None, }, @@ -650,15 +726,15 @@ mod tests { #[tokio::test] async fn test_priority_range_and_reliability_negotiation_scenario_1() { test_negotiation_ok( - QoS::Enabled { + State::QoS { reliability: Some(Reliability::BestEffort), priorities: Some(priority_range!(1, 3)), }, - QoS::Enabled { + State::QoS { reliability: Some(Reliability::BestEffort), priorities: Some(priority_range!(1, 3)), }, - QoS::Enabled { + State::QoS { reliability: Some(Reliability::BestEffort), priorities: Some(priority_range!(1, 3)), }, diff --git a/io/zenoh-transport/src/unicast/establishment/open.rs b/io/zenoh-transport/src/unicast/establishment/open.rs index 81b8ed90a9..0805dad65f 100644 --- a/io/zenoh-transport/src/unicast/establishment/open.rs +++ b/io/zenoh-transport/src/unicast/establishment/open.rs @@ -52,7 +52,7 @@ type OpenError = (zenoh_result::Error, Option); struct StateTransport { batch_size: BatchSize, resolution: Resolution, - ext_qos: ext::qos::QoS, + ext_qos: ext::qos::StateOpen, #[cfg(feature = "transport_multilink")] ext_mlink: ext::multilink::StateOpen, #[cfg(feature = "shared-memory")] @@ -586,7 +586,7 @@ pub(crate) async fn open_link( transport: StateTransport { batch_size, resolution: manager.config.resolution, - ext_qos: ext::qos::QoS::new(manager.config.unicast.is_qos, &endpoint)?, + ext_qos: ext::qos::StateOpen::new(manager.config.unicast.is_qos, &endpoint)?, #[cfg(feature = "transport_multilink")] ext_mlink: manager .state @@ -654,7 +654,7 @@ pub(crate) async fn open_link( whatami: iack_out.other_whatami, sn_resolution: state.transport.resolution.get(Field::FrameSN), tx_initial_sn: osyn_out.mine_initial_sn, - is_qos: state.transport.ext_qos.is_enabled(), + is_qos: state.transport.ext_qos.is_qos(), #[cfg(feature = "transport_multilink")] multilink: state.transport.ext_mlink.multilink(), #[cfg(feature = "shared-memory")] From fd7b050d2bc3d34843c44132033d66e58abf6cac Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Fri, 6 Sep 2024 08:02:46 +0000 Subject: [PATCH 22/37] Remove `NewLinkUnicast` --- commons/zenoh-protocol/src/core/locator.rs | 4 ++++ io/zenoh-link-commons/src/unicast.rs | 13 +------------ io/zenoh-links/zenoh-link-quic/src/unicast.rs | 8 +++----- io/zenoh-links/zenoh-link-serial/src/unicast.rs | 10 +++------- io/zenoh-links/zenoh-link-tcp/src/unicast.rs | 8 +++----- io/zenoh-links/zenoh-link-tls/src/unicast.rs | 8 +++----- io/zenoh-links/zenoh-link-udp/src/unicast.rs | 9 +++------ .../zenoh-link-unixpipe/src/unix/unicast.rs | 9 ++------- .../zenoh-link-unixsock_stream/src/unicast.rs | 7 ++----- io/zenoh-links/zenoh-link-vsock/src/unicast.rs | 7 ++----- io/zenoh-links/zenoh-link-ws/src/unicast.rs | 13 ++----------- .../src/unicast/establishment/accept.rs | 9 +++------ io/zenoh-transport/src/unicast/manager.rs | 5 ++--- 13 files changed, 33 insertions(+), 77 deletions(-) diff --git a/commons/zenoh-protocol/src/core/locator.rs b/commons/zenoh-protocol/src/core/locator.rs index 14f899e7c6..a34cc55320 100644 --- a/commons/zenoh-protocol/src/core/locator.rs +++ b/commons/zenoh-protocol/src/core/locator.rs @@ -64,6 +64,10 @@ impl Locator { pub fn as_str(&self) -> &str { self.0.as_str() } + + pub fn to_endpoint(&self) -> EndPoint { + self.0.clone() + } } impl From for Locator { diff --git a/io/zenoh-link-commons/src/unicast.rs b/io/zenoh-link-commons/src/unicast.rs index 15d7ce8b17..18ee8d2a45 100644 --- a/io/zenoh-link-commons/src/unicast.rs +++ b/io/zenoh-link-commons/src/unicast.rs @@ -36,18 +36,7 @@ pub trait LinkManagerUnicastTrait: Send + Sync { async fn get_listeners(&self) -> Vec; async fn get_locators(&self) -> Vec; } -pub type NewLinkChannelSender = flume::Sender; - -// TODO(fuzzypixelz): Delete this -/// Notification of a new inbound connection. -/// -/// Link implementations should preserve the metadata sections of [`NewLinkUnicast::endpoint`]. -pub struct NewLinkUnicast { - /// The link created in response to a new inbound connection. - pub link: LinkUnicast, - /// Endpoint of the listener. - pub endpoint: EndPoint, -} +pub type NewLinkChannelSender = flume::Sender; pub trait ConstructibleLinkManagerUnicast: Sized { fn new(new_link_sender: NewLinkChannelSender, config: T) -> ZResult; diff --git a/io/zenoh-links/zenoh-link-quic/src/unicast.rs b/io/zenoh-links/zenoh-link-quic/src/unicast.rs index 3fdcfdc62d..266cdb8e31 100644 --- a/io/zenoh-links/zenoh-link-quic/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-quic/src/unicast.rs @@ -27,7 +27,7 @@ use x509_parser::prelude::*; use zenoh_core::zasynclock; use zenoh_link_commons::{ get_ip_interface_names, LinkAuthId, LinkAuthType, LinkManagerUnicastTrait, LinkUnicast, - LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, NewLinkUnicast, + LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -336,9 +336,8 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastQuic { let task = { let token = token.clone(); let manager = self.manager.clone(); - let endpoint = endpoint.clone(); - async move { accept_task(endpoint, quic_endpoint, token, manager).await } + async move { accept_task(quic_endpoint, token, manager).await } }; // Initialize the QuicAcceptor @@ -367,7 +366,6 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastQuic { } async fn accept_task( - endpoint: EndPoint, quic_endpoint: quinn::Endpoint, token: CancellationToken, manager: NewLinkChannelSender, @@ -433,7 +431,7 @@ async fn accept_task( )); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(NewLinkUnicast { link: LinkUnicast(link), endpoint: endpoint.clone() }).await { + if let Err(e) = manager.send_async(LinkUnicast(link)).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } diff --git a/io/zenoh-links/zenoh-link-serial/src/unicast.rs b/io/zenoh-links/zenoh-link-serial/src/unicast.rs index b048121a9a..0d6d4b07ee 100644 --- a/io/zenoh-links/zenoh-link-serial/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-serial/src/unicast.rs @@ -33,7 +33,7 @@ use z_serial::ZSerial; use zenoh_core::{zasynclock, zasyncread, zasyncwrite}; use zenoh_link_commons::{ ConstructibleLinkManagerUnicast, LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, - LinkUnicastTrait, NewLinkChannelSender, NewLinkUnicast, + LinkUnicastTrait, NewLinkChannelSender, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -339,13 +339,10 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastSerial { let path = path.clone(); let manager = self.manager.clone(); let listeners = self.listeners.clone(); - let endpoint = endpoint.clone(); async move { // Wait for the accept loop to terminate - let res = - accept_read_task(endpoint, link, token, manager, path.clone(), is_connected) - .await; + let res = accept_read_task(link, token, manager, path.clone(), is_connected).await; zasyncwrite!(listeners).remove(&path); res } @@ -394,7 +391,6 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastSerial { } async fn accept_read_task( - endpoint: EndPoint, link: Arc, token: CancellationToken, manager: NewLinkChannelSender, @@ -428,7 +424,7 @@ async fn accept_read_task( match res { Ok(link) => { // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(NewLinkUnicast{ link: LinkUnicast(link.clone()), endpoint: endpoint.clone() }).await { + if let Err(e) = manager.send_async(LinkUnicast(link.clone())).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } diff --git a/io/zenoh-links/zenoh-link-tcp/src/unicast.rs b/io/zenoh-links/zenoh-link-tcp/src/unicast.rs index 419c18f163..5b7d7ab7cb 100644 --- a/io/zenoh-links/zenoh-link-tcp/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-tcp/src/unicast.rs @@ -21,7 +21,7 @@ use tokio::{ use tokio_util::sync::CancellationToken; use zenoh_link_commons::{ get_ip_interface_names, LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, LinkUnicastTrait, - ListenersUnicastIP, NewLinkChannelSender, NewLinkUnicast, BIND_INTERFACE, + ListenersUnicastIP, NewLinkChannelSender, BIND_INTERFACE, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -358,9 +358,8 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastTcp { let task = { let token = token.clone(); let manager = self.manager.clone(); - let endpoint = endpoint.clone(); - async move { accept_task(endpoint, socket, token, manager).await } + async move { accept_task(socket, token, manager).await } }; let locator = endpoint.to_locator(); @@ -425,7 +424,6 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastTcp { } async fn accept_task( - endpoint: EndPoint, socket: TcpListener, token: CancellationToken, manager: NewLinkChannelSender, @@ -462,7 +460,7 @@ async fn accept_task( let link = Arc::new(LinkUnicastTcp::new(stream, src_addr, dst_addr)); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(NewLinkUnicast{ link: LinkUnicast(link), endpoint: endpoint.clone() }).await { + if let Err(e) = manager.send_async(LinkUnicast(link)).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } }, diff --git a/io/zenoh-links/zenoh-link-tls/src/unicast.rs b/io/zenoh-links/zenoh-link-tls/src/unicast.rs index d75835e653..710b3897d6 100644 --- a/io/zenoh-links/zenoh-link-tls/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-tls/src/unicast.rs @@ -25,7 +25,7 @@ use x509_parser::prelude::*; use zenoh_core::zasynclock; use zenoh_link_commons::{ get_ip_interface_names, LinkAuthId, LinkAuthType, LinkManagerUnicastTrait, LinkUnicast, - LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, NewLinkUnicast, + LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -368,9 +368,8 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastTls { let acceptor = TlsAcceptor::from(Arc::new(tls_server_config.server_config)); let token = token.clone(); let manager = self.manager.clone(); - let endpoint = endpoint.clone(); - async move { accept_task(endpoint, socket, acceptor, token, manager).await } + async move { accept_task(socket, acceptor, token, manager).await } }; // Update the endpoint locator address @@ -403,7 +402,6 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastTls { } async fn accept_task( - endpoint: EndPoint, socket: TcpListener, acceptor: TlsAcceptor, token: CancellationToken, @@ -461,7 +459,7 @@ async fn accept_task( )); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(NewLinkUnicast {link: LinkUnicast(link), endpoint: endpoint.clone()}).await { + if let Err(e) = manager.send_async(LinkUnicast(link)).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } } diff --git a/io/zenoh-links/zenoh-link-udp/src/unicast.rs b/io/zenoh-links/zenoh-link-udp/src/unicast.rs index 0c3c6a5039..100f7dab70 100644 --- a/io/zenoh-links/zenoh-link-udp/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-udp/src/unicast.rs @@ -25,8 +25,7 @@ use tokio_util::sync::CancellationToken; use zenoh_core::{zasynclock, zlock}; use zenoh_link_commons::{ get_ip_interface_names, ConstructibleLinkManagerUnicast, LinkAuthId, LinkManagerUnicastTrait, - LinkUnicast, LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, NewLinkUnicast, - BIND_INTERFACE, + LinkUnicast, LinkUnicastTrait, ListenersUnicastIP, NewLinkChannelSender, BIND_INTERFACE, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -407,9 +406,8 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastUdp { let task = { let token = token.clone(); let manager = self.manager.clone(); - let endpoint = endpoint.clone(); - async move { accept_read_task(endpoint, socket, token, manager).await } + async move { accept_read_task(socket, token, manager).await } }; let locator = endpoint.to_locator(); @@ -476,7 +474,6 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastUdp { } async fn accept_read_task( - endpoint: EndPoint, socket: UdpSocket, token: CancellationToken, manager: NewLinkChannelSender, @@ -550,7 +547,7 @@ async fn accept_read_task( LinkUnicastUdpVariant::Unconnected(unconnected), )); // Add the new link to the set of connected peers - if let Err(e) = manager.send_async(NewLinkUnicast { link: LinkUnicast(link), endpoint: endpoint.clone() }).await { + if let Err(e) = manager.send_async(LinkUnicast(link)).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } } diff --git a/io/zenoh-links/zenoh-link-unixpipe/src/unix/unicast.rs b/io/zenoh-links/zenoh-link-unixpipe/src/unix/unicast.rs index b93bb32398..ce690ce60d 100644 --- a/io/zenoh-links/zenoh-link-unixpipe/src/unix/unicast.rs +++ b/io/zenoh-links/zenoh-link-unixpipe/src/unix/unicast.rs @@ -37,7 +37,7 @@ use unix_named_pipe::{create, open_write}; use zenoh_core::{zasyncread, zasyncwrite, ResolveFuture, Wait}; use zenoh_link_commons::{ ConstructibleLinkManagerUnicast, LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, - LinkUnicastTrait, NewLinkChannelSender, NewLinkUnicast, + LinkUnicastTrait, NewLinkChannelSender, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -281,12 +281,7 @@ async fn handle_incoming_connections( }); // send newly established link to manager - manager - .send_async(NewLinkUnicast { - link: LinkUnicast(link), - endpoint: endpoint.clone(), - }) - .await?; + manager.send_async(LinkUnicast(link)).await?; ZResult::Ok(()) } diff --git a/io/zenoh-links/zenoh-link-unixsock_stream/src/unicast.rs b/io/zenoh-links/zenoh-link-unixsock_stream/src/unicast.rs index 7bbd1f04a4..248647e7f6 100644 --- a/io/zenoh-links/zenoh-link-unixsock_stream/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-unixsock_stream/src/unicast.rs @@ -28,7 +28,6 @@ use uuid::Uuid; use zenoh_core::{zasyncread, zasyncwrite}; use zenoh_link_commons::{ LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, LinkUnicastTrait, NewLinkChannelSender, - NewLinkUnicast, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -397,11 +396,10 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastUnixSocketStream { let manager = self.manager.clone(); let listeners = self.listeners.clone(); let path = local_path_str.to_owned(); - let endpoint = endpoint.clone(); async move { // Wait for the accept loop to terminate - let res = accept_task(endpoint, socket, c_token, manager).await; + let res = accept_task(socket, c_token, manager).await; zasyncwrite!(listeners).remove(&path); res } @@ -463,7 +461,6 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastUnixSocketStream { } async fn accept_task( - endpoint: EndPoint, socket: UnixListener, token: CancellationToken, manager: NewLinkChannelSender, @@ -520,7 +517,7 @@ async fn accept_task( )); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(NewLinkUnicast { link: LinkUnicast(link), endpoint: endpoint.clone() }).await { + if let Err(e) = manager.send_async(LinkUnicast(link)).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } diff --git a/io/zenoh-links/zenoh-link-vsock/src/unicast.rs b/io/zenoh-links/zenoh-link-vsock/src/unicast.rs index 8ffb76d3ab..fb29d6539d 100644 --- a/io/zenoh-links/zenoh-link-vsock/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-vsock/src/unicast.rs @@ -29,7 +29,6 @@ use tokio_vsock::{ use zenoh_core::{zasyncread, zasyncwrite}; use zenoh_link_commons::{ LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, LinkUnicastTrait, NewLinkChannelSender, - NewLinkUnicast, }; use zenoh_protocol::{ core::{endpoint::Address, EndPoint, Locator}, @@ -282,11 +281,10 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastVsock { let token = token.clone(); let manager = self.manager.clone(); let listeners = self.listeners.clone(); - let endpoint = endpoint.clone(); async move { // Wait for the accept loop to terminate - let res = accept_task(endpoint.clone(), listener, token, manager).await; + let res = accept_task(listener, token, manager).await; zasyncwrite!(listeners).remove(&addr); res } @@ -333,7 +331,6 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastVsock { } async fn accept_task( - endpoint: EndPoint, mut socket: VsockListener, token: CancellationToken, manager: NewLinkChannelSender, @@ -361,7 +358,7 @@ async fn accept_task( let link = Arc::new(LinkUnicastVsock::new(stream, src_addr, dst_addr)); // Communicate the new link to the initial transport manager - if let Err(e) = manager.send_async(NewLinkUnicast { link: LinkUnicast(link), endpoint: endpoint.clone() }).await { + if let Err(e) = manager.send_async(LinkUnicast(link)).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } }, diff --git a/io/zenoh-links/zenoh-link-ws/src/unicast.rs b/io/zenoh-links/zenoh-link-ws/src/unicast.rs index 82dc9993b3..e16b38cc68 100644 --- a/io/zenoh-links/zenoh-link-ws/src/unicast.rs +++ b/io/zenoh-links/zenoh-link-ws/src/unicast.rs @@ -35,7 +35,6 @@ use tokio_util::sync::CancellationToken; use zenoh_core::{zasynclock, zasyncread, zasyncwrite}; use zenoh_link_commons::{ LinkAuthId, LinkManagerUnicastTrait, LinkUnicast, LinkUnicastTrait, NewLinkChannelSender, - NewLinkUnicast, }; use zenoh_protocol::{ core::{EndPoint, Locator}, @@ -375,11 +374,10 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastWs { let manager = self.manager.clone(); let listeners = self.listeners.clone(); let addr = local_addr; - let endpoint = endpoint.clone(); async move { // Wait for the accept loop to terminate - let res = accept_task(endpoint, socket, token, manager).await; + let res = accept_task(socket, token, manager).await; zasyncwrite!(listeners).remove(&addr); res } @@ -472,7 +470,6 @@ impl LinkManagerUnicastTrait for LinkManagerUnicastWs { } async fn accept_task( - endpoint: EndPoint, socket: TcpListener, token: CancellationToken, manager: NewLinkChannelSender, @@ -541,13 +538,7 @@ async fn accept_task( let link = Arc::new(LinkUnicastWs::new(stream, src_addr, dst_addr)); // Communicate the new link to the initial transport manager - if let Err(e) = manager - .send_async(NewLinkUnicast { - link: LinkUnicast(link), - endpoint: endpoint.clone(), - }) - .await - { + if let Err(e) = manager.send_async(LinkUnicast(link)).await { tracing::error!("{}-{}: {}", file!(), line!(), e) } } diff --git a/io/zenoh-transport/src/unicast/establishment/accept.rs b/io/zenoh-transport/src/unicast/establishment/accept.rs index ef95ee7557..b66b4981e3 100644 --- a/io/zenoh-transport/src/unicast/establishment/accept.rs +++ b/io/zenoh-transport/src/unicast/establishment/accept.rs @@ -20,7 +20,7 @@ use zenoh_buffers::{reader::HasReader, writer::HasWriter, ZSlice}; use zenoh_codec::{RCodec, WCodec, Zenoh080}; use zenoh_core::{zasynclock, zcondfeat, zerror}; use zenoh_crypto::{BlockCipher, PseudoRng}; -use zenoh_link::{EndPoint, LinkUnicast}; +use zenoh_link::LinkUnicast; use zenoh_protocol::{ core::{Field, Reliability, Resolution, WhatAmI, ZenohIdProto}, transport::{ @@ -641,11 +641,8 @@ impl<'a, 'b: 'a> AcceptFsm for &'a mut AcceptLink<'b> { } } -pub(crate) async fn accept_link( - endpoint: EndPoint, - link: LinkUnicast, - manager: &TransportManager, -) -> ZResult<()> { +pub(crate) async fn accept_link(link: LinkUnicast, manager: &TransportManager) -> ZResult<()> { + let endpoint = link.get_src().to_endpoint(); let direction = TransportLinkUnicastDirection::Inbound; let mtu = link.get_mtu(); let is_streamed = link.is_streamed(); diff --git a/io/zenoh-transport/src/unicast/manager.rs b/io/zenoh-transport/src/unicast/manager.rs index d0fbecad5f..b9c79b681d 100644 --- a/io/zenoh-transport/src/unicast/manager.rs +++ b/io/zenoh-transport/src/unicast/manager.rs @@ -736,8 +736,7 @@ impl TransportManager { Ok(()) } - pub(crate) async fn handle_new_link_unicast(&self, new_link: NewLinkUnicast) { - let NewLinkUnicast { link, endpoint } = new_link; + pub(crate) async fn handle_new_link_unicast(&self, link: LinkUnicast) { let incoming_counter = self.state.unicast.incoming.clone(); if incoming_counter.load(SeqCst) >= self.config.unicast.accept_pending { // We reached the limit of concurrent incoming transport, this means two things: @@ -760,7 +759,7 @@ impl TransportManager { .spawn_with_rt(zenoh_runtime::ZRuntime::Acceptor, async move { if tokio::time::timeout( c_manager.config.unicast.accept_timeout, - super::establishment::accept::accept_link(endpoint, link, &c_manager), + super::establishment::accept::accept_link(link, &c_manager), ) .await .is_err() From 2e3285d5e2d65bab5a3c5e00147db197e0884a61 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Fri, 6 Sep 2024 08:11:41 +0000 Subject: [PATCH 23/37] Fix test typos --- io/zenoh-transport/src/unicast/establishment/ext/qos.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index 36ab15b077..6f3c440886 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -629,7 +629,7 @@ mod tests { reliability: None, }, State::QoS { - priorities: Some(priority_range!(1, 3)), + priorities: Some(priority_range!(0, 4)), reliability: None, }, State::QoS { @@ -648,7 +648,7 @@ mod tests { reliability: None, }, State::QoS { - priorities: Some(priority_range!(1, 3)), + priorities: Some(priority_range!(2, 3)), reliability: None, }, ) @@ -732,7 +732,7 @@ mod tests { }, State::QoS { reliability: Some(Reliability::BestEffort), - priorities: Some(priority_range!(1, 3)), + priorities: Some(priority_range!(1, 4)), }, State::QoS { reliability: Some(Reliability::BestEffort), From 3c2d5787ef6c2a59c359d5f36e5fd381fc3a2d3b Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Fri, 6 Sep 2024 09:08:38 +0000 Subject: [PATCH 24/37] Patch `Link::src` and `Link::dst` with negotiated metadata --- commons/zenoh-protocol/src/core/endpoint.rs | 3 +++ commons/zenoh-protocol/src/core/mod.rs | 24 +++++++++++++++--- io/zenoh-link-commons/src/lib.rs | 25 ++++++++++++++++--- .../src/unicast/establishment/ext/qos.rs | 9 +++---- 4 files changed, 48 insertions(+), 13 deletions(-) diff --git a/commons/zenoh-protocol/src/core/endpoint.rs b/commons/zenoh-protocol/src/core/endpoint.rs index 96b9b40665..237842ea6e 100644 --- a/commons/zenoh-protocol/src/core/endpoint.rs +++ b/commons/zenoh-protocol/src/core/endpoint.rs @@ -187,6 +187,9 @@ impl fmt::Debug for AddressMut<'_> { pub struct Metadata<'a>(pub(super) &'a str); impl<'a> Metadata<'a> { + pub const RELIABILITY: &'static str = "reliability"; + pub const PRIORITIES: &'static str = "priorities"; + pub fn as_str(&self) -> &'a str { self.0 } diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index 02b1cf0c95..dabbafd0c8 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -351,6 +351,12 @@ impl PriorityRange { } } +impl ToString for PriorityRange { + fn to_string(&self) -> String { + format!("{}..={}", *self.start() as u8, *self.end() as u8) + } +} + #[derive(Debug)] pub enum InvalidPriorityRange { InvalidSyntax { found: String }, @@ -458,6 +464,16 @@ pub enum Reliability { impl Reliability { pub const DEFAULT: Self = Self::Reliable; + const BEST_EFFORT_STR: &str = "best_effort"; + const RELIABLE_STR: &str = "reliable"; + + pub fn as_str(&self) -> &str { + match self { + Reliability::BestEffort => Reliability::BEST_EFFORT_STR, + Reliability::Reliable => Reliability::RELIABLE_STR, + } + } + #[cfg(feature = "test")] pub fn rand() -> Self { use rand::Rng; @@ -500,7 +516,9 @@ impl Display for InvalidReliability { fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { write!( f, - "invalid Reliability string, expected `best_effort` or `reliable` but found {}", + "invalid Reliability string, expected `{}` or `{}` but found {}", + Reliability::BEST_EFFORT_STR, + Reliability::RELIABLE_STR, self.found ) } @@ -514,8 +532,8 @@ impl FromStr for Reliability { fn from_str(s: &str) -> Result { match s { - "reliable" => Ok(Reliability::Reliable), - "best_effort" => Ok(Reliability::BestEffort), + Reliability::RELIABLE_STR => Ok(Reliability::Reliable), + Reliability::BEST_EFFORT_STR => Ok(Reliability::BestEffort), other => Err(InvalidReliability { found: other.to_string(), }), diff --git a/io/zenoh-link-commons/src/lib.rs b/io/zenoh-link-commons/src/lib.rs index dd26789ff2..7f7593780a 100644 --- a/io/zenoh-link-commons/src/lib.rs +++ b/io/zenoh-link-commons/src/lib.rs @@ -34,7 +34,7 @@ pub use multicast::*; use serde::Serialize; pub use unicast::*; use zenoh_protocol::{ - core::{Locator, PriorityRange, Reliability}, + core::{Locator, Metadata, PriorityRange, Reliability}, transport::BatchSize, }; use zenoh_result::ZResult; @@ -45,7 +45,6 @@ use zenoh_result::ZResult; pub const BIND_INTERFACE: &str = "iface"; -// TODO(fuzzypixelz): Patch the Locators to contain negotiated priority and reliability #[derive(Clone, Debug, Serialize, Hash, PartialEq, Eq)] pub struct Link { pub src: Locator, @@ -82,8 +81,8 @@ impl Link { reliability: Reliability, ) -> Self { Link { - src: link.get_src().to_owned(), - dst: link.get_dst().to_owned(), + src: Self::to_patched_locator(link.get_src(), priorities.as_ref(), reliability), + dst: Self::to_patched_locator(link.get_dst(), priorities.as_ref(), reliability), group: None, mtu: link.get_mtu(), is_streamed: link.is_streamed(), @@ -107,6 +106,24 @@ impl Link { reliability: Reliability::from(link.is_reliable()), } } + + /// Updates the metdata of the `locator` with `priorities` and `reliability`. + fn to_patched_locator( + locator: &Locator, + priorities: Option<&PriorityRange>, + reliability: Reliability, + ) -> Locator { + let mut locator = locator.clone(); + let mut metadata = locator.metadata_mut(); + metadata + .insert(Metadata::RELIABILITY, reliability.as_str()) + .expect("adding `reliability` to Locator metadata should not fail"); + priorities + .map(|ps| metadata.insert(Metadata::PRIORITIES, ps.to_string())) + .transpose() + .expect("adding `priorities` to Locator metadata should not fail"); + locator + } } impl PartialEq for Link { diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index 6f3c440886..d094b7607b 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -23,7 +23,7 @@ use zenoh_codec::{RCodec, WCodec, Zenoh080}; use zenoh_core::zerror; use zenoh_link::EndPoint; use zenoh_protocol::{ - core::{Priority, PriorityRange, Reliability}, + core::{Metadata, Priority, PriorityRange, Reliability}, transport::{init, open}, }; use zenoh_result::{Error as ZError, ZResult}; @@ -56,18 +56,15 @@ impl State { if !is_qos { Ok(State::NoQoS) } else { - const RELIABILITY_METADATA_KEY: &str = "reliability"; - const PRIORITY_METADATA_KEY: &str = "priorities"; - let metadata = endpoint.metadata(); let reliability = metadata - .get(RELIABILITY_METADATA_KEY) + .get(Metadata::RELIABILITY) .map(Reliability::from_str) .transpose()?; let priorities = metadata - .get(PRIORITY_METADATA_KEY) + .get(Metadata::PRIORITIES) .map(PriorityRange::from_str) .transpose()?; From 877f61d2aca4ca2dda1e35d15f53728dbc2d070b Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Fri, 6 Sep 2024 09:40:31 +0000 Subject: [PATCH 25/37] Optimize `QoS` extension overhead --- commons/zenoh-codec/src/transport/init.rs | 24 +++++++ commons/zenoh-protocol/src/transport/init.rs | 7 +++ .../src/unicast/establishment/accept.rs | 8 ++- .../src/unicast/establishment/ext/qos.rs | 62 +++++++++++++------ .../src/unicast/establishment/open.rs | 8 ++- 5 files changed, 86 insertions(+), 23 deletions(-) diff --git a/commons/zenoh-codec/src/transport/init.rs b/commons/zenoh-codec/src/transport/init.rs index c559fdbd51..c2609f2657 100644 --- a/commons/zenoh-codec/src/transport/init.rs +++ b/commons/zenoh-codec/src/transport/init.rs @@ -45,6 +45,7 @@ where resolution, batch_size, ext_qos, + ext_qos_optimized, #[cfg(feature = "shared-memory")] ext_shm, ext_auth, @@ -98,6 +99,10 @@ where n_exts -= 1; self.write(&mut *writer, (qos, n_exts != 0))?; } + if let Some(qos_optimized) = ext_qos_optimized.as_ref() { + n_exts -= 1; + self.write(&mut *writer, (qos_optimized, n_exts != 0))?; + } #[cfg(feature = "shared-memory")] if let Some(shm) = ext_shm.as_ref() { n_exts -= 1; @@ -173,6 +178,7 @@ where // Extensions let mut ext_qos = None; + let mut ext_qos_optimized = None; #[cfg(feature = "shared-memory")] let mut ext_shm = None; let mut ext_auth = None; @@ -190,6 +196,11 @@ where ext_qos = Some(q); has_ext = ext; } + ext::QoSOptimized::ID => { + let (q, ext): (ext::QoSOptimized, bool) = eodec.read(&mut *reader)?; + ext_qos_optimized = Some(q); + has_ext = ext; + } #[cfg(feature = "shared-memory")] ext::Shm::ID => { let (s, ext): (ext::Shm, bool) = eodec.read(&mut *reader)?; @@ -229,6 +240,7 @@ where resolution, batch_size, ext_qos, + ext_qos_optimized, #[cfg(feature = "shared-memory")] ext_shm, ext_auth, @@ -255,6 +267,7 @@ where batch_size, cookie, ext_qos, + ext_qos_optimized, #[cfg(feature = "shared-memory")] ext_shm, ext_auth, @@ -311,6 +324,10 @@ where n_exts -= 1; self.write(&mut *writer, (qos, n_exts != 0))?; } + if let Some(qos_optimized) = ext_qos_optimized.as_ref() { + n_exts -= 1; + self.write(&mut *writer, (qos_optimized, n_exts != 0))?; + } #[cfg(feature = "shared-memory")] if let Some(shm) = ext_shm.as_ref() { n_exts -= 1; @@ -389,6 +406,7 @@ where // Extensions let mut ext_qos = None; + let mut ext_qos_optimized = None; #[cfg(feature = "shared-memory")] let mut ext_shm = None; let mut ext_auth = None; @@ -406,6 +424,11 @@ where ext_qos = Some(q); has_ext = ext; } + ext::QoSOptimized::ID => { + let (q, ext): (ext::QoSOptimized, bool) = eodec.read(&mut *reader)?; + ext_qos_optimized = Some(q); + has_ext = ext; + } #[cfg(feature = "shared-memory")] ext::Shm::ID => { let (s, ext): (ext::Shm, bool) = eodec.read(&mut *reader)?; @@ -446,6 +469,7 @@ where batch_size, cookie, ext_qos, + ext_qos_optimized, #[cfg(feature = "shared-memory")] ext_shm, ext_auth, diff --git a/commons/zenoh-protocol/src/transport/init.rs b/commons/zenoh-protocol/src/transport/init.rs index 0a4e97f95e..7650035d41 100644 --- a/commons/zenoh-protocol/src/transport/init.rs +++ b/commons/zenoh-protocol/src/transport/init.rs @@ -115,6 +115,7 @@ pub struct InitSyn { pub resolution: Resolution, pub batch_size: BatchSize, pub ext_qos: Option, + pub ext_qos_optimized: Option, #[cfg(feature = "shared-memory")] pub ext_shm: Option, pub ext_auth: Option, @@ -133,6 +134,7 @@ pub mod ext { /// # QoS extension /// Used to negotiate the use of QoS pub type QoS = zextz64!(0x1, false); + pub type QoSOptimized = zextunit!(0x1, false); /// # Shm extension /// Used as challenge for probing shared memory capabilities @@ -171,6 +173,7 @@ impl InitSyn { let resolution = Resolution::rand(); let batch_size: BatchSize = rng.gen(); let ext_qos = rng.gen_bool(0.5).then_some(ZExtZ64::rand()); + let ext_qos_optimized = rng.gen_bool(0.5).then_some(ZExtUnit::rand()); #[cfg(feature = "shared-memory")] let ext_shm = rng.gen_bool(0.5).then_some(ZExtZBuf::rand()); let ext_auth = rng.gen_bool(0.5).then_some(ZExtZBuf::rand()); @@ -185,6 +188,7 @@ impl InitSyn { resolution, batch_size, ext_qos, + ext_qos_optimized, #[cfg(feature = "shared-memory")] ext_shm, ext_auth, @@ -204,6 +208,7 @@ pub struct InitAck { pub batch_size: BatchSize, pub cookie: ZSlice, pub ext_qos: Option, + pub ext_qos_optimized: Option, #[cfg(feature = "shared-memory")] pub ext_shm: Option, pub ext_auth: Option, @@ -232,6 +237,7 @@ impl InitAck { let batch_size: BatchSize = rng.gen(); let cookie = ZSlice::rand(64); let ext_qos = rng.gen_bool(0.5).then_some(ZExtZ64::rand()); + let ext_qos_optimized = rng.gen_bool(0.5).then_some(ZExtUnit::rand()); #[cfg(feature = "shared-memory")] let ext_shm = rng.gen_bool(0.5).then_some(ZExtZBuf::rand()); let ext_auth = rng.gen_bool(0.5).then_some(ZExtZBuf::rand()); @@ -247,6 +253,7 @@ impl InitAck { batch_size, cookie, ext_qos, + ext_qos_optimized, #[cfg(feature = "shared-memory")] ext_shm, ext_auth, diff --git a/io/zenoh-transport/src/unicast/establishment/accept.rs b/io/zenoh-transport/src/unicast/establishment/accept.rs index b66b4981e3..91ad3cac30 100644 --- a/io/zenoh-transport/src/unicast/establishment/accept.rs +++ b/io/zenoh-transport/src/unicast/establishment/accept.rs @@ -224,7 +224,10 @@ impl<'a, 'b: 'a> AcceptFsm for &'a mut AcceptLink<'b> { // Extension QoS self.ext_qos - .recv_init_syn((&mut state.transport.ext_qos, init_syn.ext_qos)) + .recv_init_syn(( + &mut state.transport.ext_qos, + (init_syn.ext_qos, init_syn.ext_qos_optimized), + )) .await .map_err(|e| (e, Some(close::reason::GENERIC)))?; @@ -284,7 +287,7 @@ impl<'a, 'b: 'a> AcceptFsm for &'a mut AcceptLink<'b> { let (mut state, input) = input; // Extension QoS - let ext_qos = self + let (ext_qos, ext_qos_optimized) = self .ext_qos .send_init_ack(&state.transport.ext_qos) .await @@ -381,6 +384,7 @@ impl<'a, 'b: 'a> AcceptFsm for &'a mut AcceptLink<'b> { batch_size: state.transport.batch_size, cookie, ext_qos, + ext_qos_optimized, #[cfg(feature = "shared-memory")] ext_shm, ext_auth, diff --git a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs index d094b7607b..350794ff7b 100644 --- a/io/zenoh-transport/src/unicast/establishment/ext/qos.rs +++ b/io/zenoh-transport/src/unicast/establishment/ext/qos.rs @@ -41,7 +41,6 @@ impl<'a> QoSFsm<'a> { } } -// TODO(fuzzypixelz): Fallback to ZExtUnit QoS matching QoS::Disabled or QoS::Enabled { reliability: None, priorities: None } #[derive(Clone, Debug, PartialEq, Eq)] enum State { NoQoS, @@ -153,19 +152,38 @@ impl State { } } - fn to_ext(&self) -> Option { - if self.is_qos() { - Some(init::ext::QoS::new(self.to_u64())) - } else { - None + fn to_exts(&self) -> (Option, Option) { + match self { + State::NoQoS => (None, None), + State::QoS { + reliability: None, + priorities: None, + } => (None, Some(init::ext::QoSOptimized::new())), + State::QoS { + reliability: Some(_), + .. + } + | State::QoS { + priorities: Some(_), + .. + } => (Some(init::ext::QoS::new(self.to_u64())), None), } } - fn try_from_ext(ext: Option) -> ZResult { - if let Some(ext) = ext { - State::try_from_u64(ext.value) - } else { - Ok(State::NoQoS) + fn try_from_exts( + (qos, qos_optimized): (Option, Option), + ) -> ZResult { + match (qos, qos_optimized) { + (Some(_), Some(_)) => Err(zerror!( + "Extensions QoS and QoSOptimized cannot both be enabled at once" + ) + .into()), + (None, None) => Ok(State::NoQoS), + (None, Some(_)) => Ok(State::QoS { + reliability: None, + priorities: None, + }), + (Some(qos), None) => State::try_from_u64(qos.value), } } @@ -251,15 +269,18 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { type Error = ZError; type SendInitSynIn = &'a StateOpen; - type SendInitSynOut = Option; + type SendInitSynOut = (Option, Option); async fn send_init_syn( self, state: Self::SendInitSynIn, ) -> Result { - Ok(state.0.to_ext()) + Ok(state.0.to_exts()) } - type RecvInitAckIn = (&'a mut StateOpen, Option); + type RecvInitAckIn = ( + &'a mut StateOpen, + (Option, Option), + ); type RecvInitAckOut = (); async fn recv_init_ack( self, @@ -267,7 +288,7 @@ impl<'a> OpenFsm for &'a QoSFsm<'a> { ) -> Result { let (state_self, other_ext) = input; - let state_other = State::try_from_ext(other_ext)?; + let state_other = State::try_from_exts(other_ext)?; let ( State::QoS { @@ -401,7 +422,10 @@ where impl<'a> AcceptFsm for &'a QoSFsm<'a> { type Error = ZError; - type RecvInitSynIn = (&'a mut StateAccept, Option); + type RecvInitSynIn = ( + &'a mut StateAccept, + (Option, Option), + ); type RecvInitSynOut = (); async fn recv_init_syn( self, @@ -409,7 +433,7 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { ) -> Result { let (state_self, other_ext) = input; - let state_other = State::try_from_ext(other_ext)?; + let state_other = State::try_from_exts(other_ext)?; let ( State::QoS { @@ -464,12 +488,12 @@ impl<'a> AcceptFsm for &'a QoSFsm<'a> { } type SendInitAckIn = &'a StateAccept; - type SendInitAckOut = Option; + type SendInitAckOut = (Option, Option); async fn send_init_ack( self, state: Self::SendInitAckIn, ) -> Result { - Ok(state.0.to_ext()) + Ok(state.0.to_exts()) } type RecvOpenSynIn = (&'a mut StateAccept, Option); diff --git a/io/zenoh-transport/src/unicast/establishment/open.rs b/io/zenoh-transport/src/unicast/establishment/open.rs index 0805dad65f..f85cdbe9f5 100644 --- a/io/zenoh-transport/src/unicast/establishment/open.rs +++ b/io/zenoh-transport/src/unicast/establishment/open.rs @@ -139,7 +139,7 @@ impl<'a, 'b: 'a> OpenFsm for &'a mut OpenLink<'b> { let (link, state, input) = input; // Extension QoS - let ext_qos = self + let (ext_qos, ext_qos_optimized) = self .ext_qos .send_init_syn(&state.transport.ext_qos) .await @@ -199,6 +199,7 @@ impl<'a, 'b: 'a> OpenFsm for &'a mut OpenLink<'b> { batch_size: state.transport.batch_size, resolution: state.transport.resolution, ext_qos, + ext_qos_optimized, #[cfg(feature = "shared-memory")] ext_shm, ext_auth, @@ -302,7 +303,10 @@ impl<'a, 'b: 'a> OpenFsm for &'a mut OpenLink<'b> { // Extension QoS self.ext_qos - .recv_init_ack((&mut state.transport.ext_qos, init_ack.ext_qos)) + .recv_init_ack(( + &mut state.transport.ext_qos, + (init_ack.ext_qos, init_ack.ext_qos_optimized), + )) .await .map_err(|e| (e, Some(close::reason::GENERIC)))?; From 50a43331acb167fffda5c36a3c7d3768937986af Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Fri, 6 Sep 2024 12:25:04 +0000 Subject: [PATCH 26/37] Implement `Display` instead of `ToString` for `PriorityRange` --- commons/zenoh-protocol/src/core/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index dabbafd0c8..08a363dc1d 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -351,9 +351,9 @@ impl PriorityRange { } } -impl ToString for PriorityRange { - fn to_string(&self) -> String { - format!("{}..={}", *self.start() as u8, *self.end() as u8) +impl Display for PriorityRange { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + write!(f, "{}..={}", *self.start() as u8, *self.end() as u8) } } From d07275a69dba8fe6710c67b5ebb9e102c942b650 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Fri, 6 Sep 2024 12:26:26 +0000 Subject: [PATCH 27/37] Fix typo (metdata -> metadata) --- io/zenoh-link-commons/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/io/zenoh-link-commons/src/lib.rs b/io/zenoh-link-commons/src/lib.rs index 7f7593780a..dcbb1c3c18 100644 --- a/io/zenoh-link-commons/src/lib.rs +++ b/io/zenoh-link-commons/src/lib.rs @@ -107,7 +107,7 @@ impl Link { } } - /// Updates the metdata of the `locator` with `priorities` and `reliability`. + /// Updates the metadata of the `locator` with `priorities` and `reliability`. fn to_patched_locator( locator: &Locator, priorities: Option<&PriorityRange>, From 5083b94030df03c311787858e0a0b5162799e238 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Fri, 6 Sep 2024 12:36:20 +0000 Subject: [PATCH 28/37] Fix `n_exts` in `INIT` codec --- commons/zenoh-codec/src/transport/init.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/commons/zenoh-codec/src/transport/init.rs b/commons/zenoh-codec/src/transport/init.rs index c2609f2657..9185955759 100644 --- a/commons/zenoh-codec/src/transport/init.rs +++ b/commons/zenoh-codec/src/transport/init.rs @@ -60,6 +60,7 @@ where header |= flag::S; } let mut n_exts = (ext_qos.is_some() as u8) + + (ext_qos_optimized.is_some() as u8) + (ext_auth.is_some() as u8) + (ext_mlink.is_some() as u8) + (ext_lowlatency.is_some() as u8) @@ -282,6 +283,7 @@ where header |= flag::S; } let mut n_exts = (ext_qos.is_some() as u8) + + (ext_qos_optimized.is_some() as u8) + (ext_auth.is_some() as u8) + (ext_mlink.is_some() as u8) + (ext_lowlatency.is_some() as u8) From ed4b555af458d210a4af9a1649c20949bd0d9529 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Fri, 6 Sep 2024 12:45:12 +0000 Subject: [PATCH 29/37] Add missing `'static` lifetime in const --- commons/zenoh-protocol/src/core/mod.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index 08a363dc1d..f55c19b9c6 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -464,8 +464,8 @@ pub enum Reliability { impl Reliability { pub const DEFAULT: Self = Self::Reliable; - const BEST_EFFORT_STR: &str = "best_effort"; - const RELIABLE_STR: &str = "reliable"; + const BEST_EFFORT_STR: &'static str = "best_effort"; + const RELIABLE_STR: &'static str = "reliable"; pub fn as_str(&self) -> &str { match self { From 4ec1396c8d0e6fd1fb42d25626823a1c3a61c2a6 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Mon, 9 Sep 2024 11:11:42 +0000 Subject: [PATCH 30/37] Don't compare `Link` to `TransportLinkUnicast` --- .../src/unicast/universal/transport.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/io/zenoh-transport/src/unicast/universal/transport.rs b/io/zenoh-transport/src/unicast/universal/transport.rs index 47f2ff344c..f01a4a8f18 100644 --- a/io/zenoh-transport/src/unicast/universal/transport.rs +++ b/io/zenoh-transport/src/unicast/universal/transport.rs @@ -42,13 +42,6 @@ use crate::{ TransportManager, TransportPeerEventHandler, }; -macro_rules! zlinkindex { - ($guard:expr, $link:expr) => { - // Compare LinkUnicast link to not compare TransportLinkUnicast direction - $guard.iter().position(|tl| tl.link == $link) - }; -} - /*************************************/ /* UNIVERSAL TRANSPORT */ /*************************************/ @@ -175,7 +168,15 @@ impl TransportUnicastUniversal { let target = { let mut guard = zwrite!(self.links); - if let Some(index) = zlinkindex!(guard, link) { + if let Some(index) = guard.iter().position(|tl| { + // Compare LinkUnicast link to not compare TransportLinkUnicast direction + Link::new_unicast( + &tl.link.link, + tl.link.config.priorities.clone(), + tl.link.config.reliability, + ) + .eq(&link) + }) { let is_last = guard.len() == 1; if is_last { // Close the whole transport From 3e321856f0b69a7fd6a8c101ed36a6ea2ad599af Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Mon, 9 Sep 2024 12:24:33 +0000 Subject: [PATCH 31/37] Don't set Link Reliability if not configured --- io/zenoh-link-commons/src/lib.rs | 19 +++++++------------ .../src/unicast/establishment/accept.rs | 10 +++------- .../src/unicast/establishment/open.rs | 10 +++------- io/zenoh-transport/src/unicast/link.rs | 2 +- .../src/unicast/universal/tx.rs | 5 ++++- 5 files changed, 18 insertions(+), 28 deletions(-) diff --git a/io/zenoh-link-commons/src/lib.rs b/io/zenoh-link-commons/src/lib.rs index dcbb1c3c18..9ca18f8c0e 100644 --- a/io/zenoh-link-commons/src/lib.rs +++ b/io/zenoh-link-commons/src/lib.rs @@ -55,7 +55,7 @@ pub struct Link { pub interfaces: Vec, pub auth_identifier: LinkAuthId, pub priorities: Option, - pub reliability: Reliability, + pub reliability: Option, } #[async_trait] @@ -78,7 +78,7 @@ impl Link { pub fn new_unicast( link: &LinkUnicast, priorities: Option, - reliability: Reliability, + reliability: Option, ) -> Self { Link { src: Self::to_patched_locator(link.get_src(), priorities.as_ref(), reliability), @@ -103,7 +103,7 @@ impl Link { interfaces: vec![], auth_identifier: LinkAuthId::default(), priorities: None, - reliability: Reliability::from(link.is_reliable()), + reliability: None, } } @@ -111,12 +111,13 @@ impl Link { fn to_patched_locator( locator: &Locator, priorities: Option<&PriorityRange>, - reliability: Reliability, + reliability: Option, ) -> Locator { let mut locator = locator.clone(); let mut metadata = locator.metadata_mut(); - metadata - .insert(Metadata::RELIABILITY, reliability.as_str()) + reliability + .map(|r| metadata.insert(Metadata::RELIABILITY, r.as_str())) + .transpose() .expect("adding `reliability` to Locator metadata should not fail"); priorities .map(|ps| metadata.insert(Metadata::PRIORITIES, ps.to_string())) @@ -126,12 +127,6 @@ impl Link { } } -impl PartialEq for Link { - fn eq(&self, other: &LinkUnicast) -> bool { - self.src == *other.get_src() && self.dst == *other.get_dst() - } -} - impl PartialEq for Link { fn eq(&self, other: &LinkMulticast) -> bool { self.src == *other.get_src() && self.dst == *other.get_dst() diff --git a/io/zenoh-transport/src/unicast/establishment/accept.rs b/io/zenoh-transport/src/unicast/establishment/accept.rs index 91ad3cac30..d344a0c8d0 100644 --- a/io/zenoh-transport/src/unicast/establishment/accept.rs +++ b/io/zenoh-transport/src/unicast/establishment/accept.rs @@ -22,7 +22,7 @@ use zenoh_core::{zasynclock, zcondfeat, zerror}; use zenoh_crypto::{BlockCipher, PseudoRng}; use zenoh_link::LinkUnicast; use zenoh_protocol::{ - core::{Field, Reliability, Resolution, WhatAmI, ZenohIdProto}, + core::{Field, Resolution, WhatAmI, ZenohIdProto}, transport::{ batch_size, close::{self, Close}, @@ -659,7 +659,7 @@ pub(crate) async fn accept_link(link: LinkUnicast, manager: &TransportManager) - is_compression: false, }, priorities: None, - reliability: Reliability::from(link.is_reliable()), + reliability: None, }; let mut link = TransportLinkUnicast::new(link, config); let mut fsm = AcceptLink { @@ -797,11 +797,7 @@ pub(crate) async fn accept_link(link: LinkUnicast, manager: &TransportManager) - is_compression: state.link.ext_compression.is_compression(), }, priorities: state.transport.ext_qos.priorities(), - reliability: state - .transport - .ext_qos - .reliability() - .unwrap_or_else(|| Reliability::from(link.link.is_reliable())), + reliability: state.transport.ext_qos.reliability(), }; let a_link = link.reconfigure(a_config); let s_link = format!("{:?}", a_link); diff --git a/io/zenoh-transport/src/unicast/establishment/open.rs b/io/zenoh-transport/src/unicast/establishment/open.rs index f85cdbe9f5..3e6d49d85f 100644 --- a/io/zenoh-transport/src/unicast/establishment/open.rs +++ b/io/zenoh-transport/src/unicast/establishment/open.rs @@ -20,7 +20,7 @@ use zenoh_core::zasynclock; use zenoh_core::{zcondfeat, zerror}; use zenoh_link::{EndPoint, LinkUnicast}; use zenoh_protocol::{ - core::{Field, Reliability, Resolution, WhatAmI, ZenohIdProto}, + core::{Field, Resolution, WhatAmI, ZenohIdProto}, transport::{ batch_size, close, BatchSize, Close, InitSyn, OpenSyn, TransportBody, TransportMessage, TransportSn, @@ -556,7 +556,7 @@ pub(crate) async fn open_link( is_compression: false, // Perform the exchange Init/Open exchange with no compression }, priorities: None, - reliability: Reliability::from(link.is_reliable()), + reliability: None, }; let mut link = TransportLinkUnicast::new(link, config); let mut fsm = OpenLink { @@ -680,11 +680,7 @@ pub(crate) async fn open_link( is_compression: state.link.ext_compression.is_compression(), }, priorities: state.transport.ext_qos.priorities(), - reliability: state - .transport - .ext_qos - .reliability() - .unwrap_or_else(|| Reliability::from(link.link.is_reliable())), + reliability: state.transport.ext_qos.reliability(), }; let o_link = link.reconfigure(o_config); let s_link = format!("{:?}", o_link); diff --git a/io/zenoh-transport/src/unicast/link.rs b/io/zenoh-transport/src/unicast/link.rs index 73862821df..8b1b3004a9 100644 --- a/io/zenoh-transport/src/unicast/link.rs +++ b/io/zenoh-transport/src/unicast/link.rs @@ -36,7 +36,7 @@ pub(crate) struct TransportLinkUnicastConfig { pub(crate) direction: TransportLinkUnicastDirection, pub(crate) batch: BatchConfig, pub(crate) priorities: Option, - pub(crate) reliability: Reliability, + pub(crate) reliability: Option, } #[derive(Clone, PartialEq, Eq)] diff --git a/io/zenoh-transport/src/unicast/universal/tx.rs b/io/zenoh-transport/src/unicast/universal/tx.rs index 27e721227c..cfad30771d 100644 --- a/io/zenoh-transport/src/unicast/universal/tx.rs +++ b/io/zenoh-transport/src/unicast/universal/tx.rs @@ -76,7 +76,10 @@ impl TransportUnicastUniversal { let Some(transport_link_index) = Self::select( transport_links.iter().map(|tl| { ( - tl.link.config.reliability, + tl.link + .config + .reliability + .unwrap_or(Reliability::from(tl.link.link.is_reliable())), tl.link.config.priorities.clone(), ) }), From 8889d1aaab792f450081ab7b8c7909efb905aaf9 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Mon, 16 Sep 2024 15:13:09 +0200 Subject: [PATCH 32/37] Update DEFAULT_CONFIG --- DEFAULT_CONFIG.json5 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/DEFAULT_CONFIG.json5 b/DEFAULT_CONFIG.json5 index 90eda6d324..7b254f25d1 100644 --- a/DEFAULT_CONFIG.json5 +++ b/DEFAULT_CONFIG.json5 @@ -340,7 +340,7 @@ /// /// ## Endpoint metadata /// - /// **priorities**: a range bounded inclusively below and above (e.g. `2..4` signifies + /// **priorities**: a range bounded inclusively below and above (e.g. `2..=4` signifies /// priorities 2, 3 and 4). This value is used to select the link used for transmission based /// on the Priority of the message in question. /// From c91a052e1f83dc66954743bb304016c47375f052 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Tue, 17 Sep 2024 09:55:23 +0000 Subject: [PATCH 33/37] Move metadata docs to `Endpoint` --- DEFAULT_CONFIG.json5 | 13 ++++--------- commons/zenoh-protocol/src/core/endpoint.rs | 11 ++++++++++- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/DEFAULT_CONFIG.json5 b/DEFAULT_CONFIG.json5 index 7b254f25d1..567b6559cf 100644 --- a/DEFAULT_CONFIG.json5 +++ b/DEFAULT_CONFIG.json5 @@ -30,6 +30,8 @@ /// The list of endpoints to connect to. /// Accepts a single list (e.g. endpoints: ["tcp/10.10.10.10:7447", "tcp/11.11.11.11:7447"]) /// or different lists for router, peer and client (e.g. endpoints: { router: ["tcp/10.10.10.10:7447"], peer: ["tcp/11.11.11.11:7447"] }). + /// + /// See https://docs.rs/zenoh/latest/zenoh/prelude/struct.EndPoint.html endpoints: [ // "/
" ], @@ -67,6 +69,8 @@ /// The list of endpoints to listen on. /// Accepts a single list (e.g. endpoints: ["tcp/[::]:7447", "udp/[::]:7447"]) /// or different lists for router, peer and client (e.g. endpoints: { router: ["tcp/[::]:7447"], peer: ["tcp/[::]:0"] }). + /// + /// See https://docs.rs/zenoh/latest/zenoh/prelude/struct.EndPoint.html endpoints: { router: ["tcp/[::]:7447"], peer: ["tcp/[::]:0"] }, /// Global listen configuration, @@ -338,15 +342,6 @@ /// protocols are: ["tcp" , "udp", "tls", "quic", "ws", "unixsock-stream", "vsock"] For /// example, to only enable "tls" and "quic": protocols: ["tls", "quic"], /// - /// ## Endpoint metadata - /// - /// **priorities**: a range bounded inclusively below and above (e.g. `2..=4` signifies - /// priorities 2, 3 and 4). This value is used to select the link used for transmission based - /// on the Priority of the message in question. - /// - /// **reliability**: either "best_effort" or "reliable". This value is used to select the link - /// used for transmission based on the Reliability of the message in question. - /// /// Configure the zenoh TX parameters of a link tx: { /// The resolution in bits to be used for the message sequence numbers. diff --git a/commons/zenoh-protocol/src/core/endpoint.rs b/commons/zenoh-protocol/src/core/endpoint.rs index 237842ea6e..80dceb2e26 100644 --- a/commons/zenoh-protocol/src/core/endpoint.rs +++ b/commons/zenoh-protocol/src/core/endpoint.rs @@ -447,9 +447,18 @@ impl fmt::Debug for ConfigMut<'_> { /// A string that respects the [`EndPoint`] canon form: `[#]`. /// /// `` is a valid [`Locator`] and `` is of the form `=;...;=` where keys are alphabetically sorted. -/// `` is optional and can be provided to configure some aspectes for an [`EndPoint`], e.g. the interface to listen on or connect to. +/// `` is optional and can be provided to configure some aspects for an [`EndPoint`], e.g. the interface to listen on or connect to. /// /// A full [`EndPoint`] string is hence in the form of `/
[?][#config]`. +/// +/// ## Metadata +/// +/// - **`priorities`**: a range bounded inclusively below and above (e.g. `2..=4` signifies +/// priorities 2, 3 and 4). This value is used to select the link used for transmission based +/// on the Priority of the message in question. +/// +/// - **`reliability`**: either "best_effort" or "reliable". This value is used to select the link +/// used for transmission based on the Reliability of the message in question. #[derive(Clone, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] #[serde(into = "String")] #[serde(try_from = "String")] From 00ae24b96e3e7ebe2ee018f6ad9823d8a3d68282 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Tue, 17 Sep 2024 10:02:51 +0000 Subject: [PATCH 34/37] Add Endpoint examples --- commons/zenoh-protocol/src/core/endpoint.rs | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/commons/zenoh-protocol/src/core/endpoint.rs b/commons/zenoh-protocol/src/core/endpoint.rs index 80dceb2e26..83599d319d 100644 --- a/commons/zenoh-protocol/src/core/endpoint.rs +++ b/commons/zenoh-protocol/src/core/endpoint.rs @@ -446,19 +446,29 @@ impl fmt::Debug for ConfigMut<'_> { /// A string that respects the [`EndPoint`] canon form: `[#]`. /// -/// `` is a valid [`Locator`] and `` is of the form `=;...;=` where keys are alphabetically sorted. -/// `` is optional and can be provided to configure some aspects for an [`EndPoint`], e.g. the interface to listen on or connect to. +/// `` is a valid [`Locator`] and `` is of the form +/// `=;...;=` where keys are alphabetically sorted. `` is +/// optional and can be provided to configure some aspects for an [`EndPoint`], e.g. the interface +/// to listen on or connect to. /// /// A full [`EndPoint`] string is hence in the form of `/
[?][#config]`. /// /// ## Metadata /// /// - **`priorities`**: a range bounded inclusively below and above (e.g. `2..=4` signifies -/// priorities 2, 3 and 4). This value is used to select the link used for transmission based -/// on the Priority of the message in question. +/// priorities 2, 3 and 4). This value is used to select the link used for transmission based on the +/// Priority of the message in question. +/// +/// For example, `tcp/localhost:7447?priorities=1..=3` assigns priorities +/// [`crate::core::Priority::RealTime`], [`crate::core::Priority::InteractiveHigh`] and +/// [`crate::core::Priority::InteractiveLow`] to the established link. /// /// - **`reliability`**: either "best_effort" or "reliable". This value is used to select the link /// used for transmission based on the Reliability of the message in question. +/// +/// For example, `tcp/localhost:7447?priorities=6..=7;reliability=best_effort` assigns priorities +/// [`crate::core::Priority::DataLow`] and [`crate::core::Priority::Background`], and +/// [`crate::core::Reliability::BestEffort`] to the established link. #[derive(Clone, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] #[serde(into = "String")] #[serde(try_from = "String")] From e14c0231e55ceb2afc1c7380d8037f94e5f78c23 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Tue, 17 Sep 2024 15:08:26 +0000 Subject: [PATCH 35/37] Fix doc list items without indentation --- commons/zenoh-protocol/src/core/endpoint.rs | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/commons/zenoh-protocol/src/core/endpoint.rs b/commons/zenoh-protocol/src/core/endpoint.rs index 83599d319d..635099009d 100644 --- a/commons/zenoh-protocol/src/core/endpoint.rs +++ b/commons/zenoh-protocol/src/core/endpoint.rs @@ -456,19 +456,19 @@ impl fmt::Debug for ConfigMut<'_> { /// ## Metadata /// /// - **`priorities`**: a range bounded inclusively below and above (e.g. `2..=4` signifies -/// priorities 2, 3 and 4). This value is used to select the link used for transmission based on the -/// Priority of the message in question. +/// priorities 2, 3 and 4). This value is used to select the link used for transmission based on the +/// Priority of the message in question. /// /// For example, `tcp/localhost:7447?priorities=1..=3` assigns priorities -/// [`crate::core::Priority::RealTime`], [`crate::core::Priority::InteractiveHigh`] and -/// [`crate::core::Priority::InteractiveLow`] to the established link. +/// [`crate::core::Priority::RealTime`], [`crate::core::Priority::InteractiveHigh`] and +/// [`crate::core::Priority::InteractiveLow`] to the established link. /// /// - **`reliability`**: either "best_effort" or "reliable". This value is used to select the link -/// used for transmission based on the Reliability of the message in question. +/// used for transmission based on the Reliability of the message in question. /// /// For example, `tcp/localhost:7447?priorities=6..=7;reliability=best_effort` assigns priorities -/// [`crate::core::Priority::DataLow`] and [`crate::core::Priority::Background`], and -/// [`crate::core::Reliability::BestEffort`] to the established link. +/// [`crate::core::Priority::DataLow`] and [`crate::core::Priority::Background`], and +/// [`crate::core::Reliability::BestEffort`] to the established link. #[derive(Clone, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] #[serde(into = "String")] #[serde(try_from = "String")] From a80e035280da29b61fe8303707baaccb01372013 Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Fri, 20 Sep 2024 14:43:16 +0000 Subject: [PATCH 36/37] Update Endpoint links in DEFAULT_CONFIG --- DEFAULT_CONFIG.json5 | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/DEFAULT_CONFIG.json5 b/DEFAULT_CONFIG.json5 index 567b6559cf..cd05f4ae01 100644 --- a/DEFAULT_CONFIG.json5 +++ b/DEFAULT_CONFIG.json5 @@ -31,7 +31,7 @@ /// Accepts a single list (e.g. endpoints: ["tcp/10.10.10.10:7447", "tcp/11.11.11.11:7447"]) /// or different lists for router, peer and client (e.g. endpoints: { router: ["tcp/10.10.10.10:7447"], peer: ["tcp/11.11.11.11:7447"] }). /// - /// See https://docs.rs/zenoh/latest/zenoh/prelude/struct.EndPoint.html + /// See https://docs.rs/zenoh/latest/zenoh/config/struct.EndPoint.html endpoints: [ // "/
" ], @@ -70,7 +70,7 @@ /// Accepts a single list (e.g. endpoints: ["tcp/[::]:7447", "udp/[::]:7447"]) /// or different lists for router, peer and client (e.g. endpoints: { router: ["tcp/[::]:7447"], peer: ["tcp/[::]:0"] }). /// - /// See https://docs.rs/zenoh/latest/zenoh/prelude/struct.EndPoint.html + /// See https://docs.rs/zenoh/latest/zenoh/config/struct.EndPoint.html endpoints: { router: ["tcp/[::]:7447"], peer: ["tcp/[::]:0"] }, /// Global listen configuration, From b18e8d55d3d79348f3a1470ab35e0923c7ff2a1c Mon Sep 17 00:00:00 2001 From: Mahmoud Mazouz Date: Mon, 23 Sep 2024 11:51:35 +0000 Subject: [PATCH 37/37] Change `x..=y` syntax to `x-y` --- commons/zenoh-protocol/src/core/endpoint.rs | 6 +++--- commons/zenoh-protocol/src/core/mod.rs | 3 ++- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/commons/zenoh-protocol/src/core/endpoint.rs b/commons/zenoh-protocol/src/core/endpoint.rs index 635099009d..5f6ac1c38f 100644 --- a/commons/zenoh-protocol/src/core/endpoint.rs +++ b/commons/zenoh-protocol/src/core/endpoint.rs @@ -455,18 +455,18 @@ impl fmt::Debug for ConfigMut<'_> { /// /// ## Metadata /// -/// - **`priorities`**: a range bounded inclusively below and above (e.g. `2..=4` signifies +/// - **`priorities`**: a range bounded inclusively below and above (e.g. `2-4` signifies /// priorities 2, 3 and 4). This value is used to select the link used for transmission based on the /// Priority of the message in question. /// -/// For example, `tcp/localhost:7447?priorities=1..=3` assigns priorities +/// For example, `tcp/localhost:7447?priorities=1-3` assigns priorities /// [`crate::core::Priority::RealTime`], [`crate::core::Priority::InteractiveHigh`] and /// [`crate::core::Priority::InteractiveLow`] to the established link. /// /// - **`reliability`**: either "best_effort" or "reliable". This value is used to select the link /// used for transmission based on the Reliability of the message in question. /// -/// For example, `tcp/localhost:7447?priorities=6..=7;reliability=best_effort` assigns priorities +/// For example, `tcp/localhost:7447?priorities=6-7;reliability=best_effort` assigns priorities /// [`crate::core::Priority::DataLow`] and [`crate::core::Priority::Background`], and /// [`crate::core::Reliability::BestEffort`] to the established link. #[derive(Clone, PartialEq, Eq, Hash, serde::Serialize, serde::Deserialize)] diff --git a/commons/zenoh-protocol/src/core/mod.rs b/commons/zenoh-protocol/src/core/mod.rs index f55c19b9c6..1c15f3830e 100644 --- a/commons/zenoh-protocol/src/core/mod.rs +++ b/commons/zenoh-protocol/src/core/mod.rs @@ -379,7 +379,8 @@ impl FromStr for PriorityRange { type Err = InvalidPriorityRange; fn from_str(s: &str) -> Result { - let mut metadata = s.split("..="); + const SEPARATOR: &str = "-"; + let mut metadata = s.split(SEPARATOR); let start = metadata .next()