From 71f60c45513f1e25303605640b8cf33cfb015579 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 22 Nov 2023 18:49:43 +0100 Subject: [PATCH 01/39] feat: dynamic stream window --- test-harness/benches/concurrent.rs | 23 +++---- test-harness/tests/ack_backlog.rs | 4 +- yamux/Cargo.toml | 3 +- yamux/src/connection.rs | 102 ++++++++++++++++++++++++++--- yamux/src/connection/stream.rs | 54 +++++++++------ yamux/src/frame.rs | 16 +++++ yamux/src/lib.rs | 5 +- 7 files changed, 159 insertions(+), 48 deletions(-) diff --git a/test-harness/benches/concurrent.rs b/test-harness/benches/concurrent.rs index e619de40..b204764a 100644 --- a/test-harness/benches/concurrent.rs +++ b/test-harness/benches/concurrent.rs @@ -8,9 +8,9 @@ // at https://www.apache.org/licenses/LICENSE-2.0 and a copy of the MIT license // at https://opensource.org/licenses/MIT. -use constrained_connection::{new_unconstrained_connection, samples, Endpoint}; +use constrained_connection::{new_unconstrained_connection, samples, Endpoint, new_constrained_connection}; use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; -use std::iter; +use std::{iter, time::Duration}; use std::sync::Arc; use test_harness::{dev_null_server, MessageSender, MessageSenderStrategy, Msg}; use tokio::{runtime::Runtime, task}; @@ -29,26 +29,19 @@ impl AsRef<[u8]> for Bytes { } fn concurrent(c: &mut Criterion) { - let data = Bytes(Arc::new(vec![0x42; 4096])); + let _ = env_logger::try_init(); + + let data = Bytes(Arc::new(vec![0x42; 10*1024*1024*1024])); let networks = vec![ - ("mobile", (|| samples::mobile_hsdpa().2) as fn() -> (_, _)), - ( - "adsl2+", - (|| samples::residential_adsl2().2) as fn() -> (_, _), - ), - ("gbit-lan", (|| samples::gbit_lan().2) as fn() -> (_, _)), - ( - "unconstrained", - new_unconstrained_connection as fn() -> (_, _), - ), + ("aws-east-west", (|| new_constrained_connection(10*1000*1000*1000, Duration::from_millis(60))) as fn() -> (_, _)), ]; let mut group = c.benchmark_group("concurrent"); group.sample_size(10); for (network_name, new_connection) in networks.into_iter() { - for nstreams in [1, 10, 100].iter() { - for nmessages in [1, 10, 100].iter() { + for nstreams in [1].iter() { + for nmessages in [1].iter() { let data = data.clone(); let rt = Runtime::new().unwrap(); diff --git a/test-harness/tests/ack_backlog.rs b/test-harness/tests/ack_backlog.rs index 2aa4e33a..30030425 100644 --- a/test-harness/tests/ack_backlog.rs +++ b/test-harness/tests/ack_backlog.rs @@ -197,8 +197,8 @@ where this.worker_streams.push(ping_pong(stream.unwrap()).boxed()); continue; } - (Poll::Ready(_), Some(_)) => { - panic!("should not be able to open stream if server hasn't acknowledged existing streams") + (Poll::Ready(e), Some(_)) => { + panic!("should not be able to open stream if server hasn't acknowledged existing streams: {:?}", e) } (Poll::Pending, None) => {} } diff --git a/yamux/Cargo.toml b/yamux/Cargo.toml index 69973e05..103a99c9 100644 --- a/yamux/Cargo.toml +++ b/yamux/Cargo.toml @@ -10,7 +10,8 @@ repository = "https://github.com/paritytech/yamux" edition = "2021" [dependencies] -futures = { version = "0.3.12", default-features = false, features = ["std"] } +futures = { version = "0.3.12", default-features = false, features = ["std", "executor"] } +futures-timer = "3" log = "0.4.8" nohash-hasher = "0.2" parking_lot = "0.12" diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 0474f79d..5e6905ec 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -32,6 +32,7 @@ use nohash_hasher::IntMap; use parking_lot::Mutex; use std::collections::VecDeque; use std::task::{Context, Waker}; +use std::time::{Duration, Instant}; use std::{fmt, sync::Arc, task::Poll}; pub use stream::{Packet, State, Stream}; @@ -287,6 +288,80 @@ struct Active { pending_frames: VecDeque>, new_outbound_stream_waker: Option, + + rtt: Rtt, +} + +#[derive(Clone, Debug)] +struct Rtt(Arc>); + +impl Rtt { + pub fn new() -> Self { + Self(Arc::new(Mutex::new(RttInner { + rtt: None, + state: RttState::Initial, + }))) + } + + fn next_ping(&mut self) -> Option> { + let state = &mut self.0.lock().state; + match state { + RttState::Initial => {} + RttState::AwaitingPong { .. } => return None, + RttState::Waiting { next } => { + // TODO: Might be expensive in the hot path. Maybe don't put at the beginning of the overall poll? + if *next > Instant::now() { + return None; + } + } + } + + let nonce = 42; + + *state = RttState::AwaitingPong { + sent_at: Instant::now(), + nonce, + }; + // TODO: randomize nonce. + Some(Frame::ping(nonce)) + } + + fn received_pong(&mut self, nonce: u32) { + let inner = &mut self.0.lock(); + match inner.state { + RttState::Initial => todo!(), + RttState::AwaitingPong { sent_at, nonce: n } => { + if nonce != n { + todo!() + } + + inner.rtt = Some(sent_at.elapsed()); + println!("{}", inner.rtt.as_ref().unwrap().as_secs_f64()); + // TODO + inner.state = RttState::Waiting { + next: Instant::now() + Duration::from_secs(10), + }; + } + RttState::Waiting { next } => todo!(), + } + } + + fn rtt(&self) -> Option { + self.0.lock().rtt + } +} + +#[derive(Debug)] +struct RttInner { + state: RttState, + rtt: Option, +} + +#[derive(Debug)] +enum RttState { + Initial, + AwaitingPong { sent_at: Instant, nonce: u32 }, + Waiting { next: Instant }, } /// `Stream` to `Connection` commands. @@ -356,6 +431,7 @@ impl Active { }, pending_frames: VecDeque::default(), new_outbound_stream_waker: None, + rtt: Rtt::new(), } } @@ -376,6 +452,11 @@ impl Active { fn poll(&mut self, cx: &mut Context<'_>) -> Poll> { loop { if self.socket.poll_ready_unpin(cx).is_ready() { + if let Some(frame) = self.rtt.next_ping() { + log::debug!("sending ping"); + self.socket.start_send_unpin(frame.into())?; + continue; + } if let Some(frame) = self.pending_frames.pop_front() { self.socket.start_send_unpin(frame)?; continue; @@ -537,7 +618,10 @@ impl Active { fn on_frame(&mut self, frame: Frame<()>) -> Result> { log::trace!("{}: received: {}", self.id, frame.header()); - if frame.header().flags().contains(header::ACK) { + // TODO: Clean this change up. + if frame.header().flags().contains(header::ACK) + && (frame.header().tag() == Tag::Data || frame.header().tag() == Tag::WindowUpdate) + { let id = frame.header().stream_id(); if let Some(stream) = self.streams.get(&id) { stream @@ -647,12 +731,12 @@ impl Active { if is_finish { shared.update_state(self.id, stream_id, State::RecvClosed); } - let max_buffer_size = self.config.max_buffer_size; + // TODO + let max_buffer_size = shared.window_max as usize; if shared.buffer.len() >= max_buffer_size { - log::error!( + panic!( "{}/{}: buffer of stream grows beyond limit", - self.id, - stream_id + self.id, stream_id ); let mut header = Header::data(stream_id, 0); header.rst(); @@ -761,7 +845,7 @@ impl Active { fn on_ping(&mut self, frame: &Frame) -> Action { let stream_id = frame.header().stream_id(); if frame.header().flags().contains(header::ACK) { - // pong + self.rtt.received_pong(frame.nonce()); return Action::None; } if stream_id == CONNECTION_ID || self.streams.contains_key(&stream_id) { @@ -769,7 +853,7 @@ impl Active { hdr.ack(); return Action::Ping(Frame::new(hdr)); } - log::trace!( + log::debug!( "{}/{}: ping for unknown stream, possibly dropped earlier: {:?}", self.id, stream_id, @@ -794,7 +878,7 @@ impl Active { waker.wake(); } - Stream::new_inbound(id, self.id, config, credit, sender) + Stream::new_inbound(id, self.id, config, credit, sender, self.rtt.clone()) } fn make_new_outbound_stream(&mut self, id: StreamId, window: u32) -> Stream { @@ -806,7 +890,7 @@ impl Active { waker.wake(); } - Stream::new_outbound(id, self.id, config, window, sender) + Stream::new_outbound(id, self.id, config, window, sender, self.rtt.clone()) } fn next_stream_id(&mut self) -> Result { diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 84ab08f8..826285a4 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -11,7 +11,7 @@ use crate::frame::header::ACK; use crate::{ chunks::Chunks, - connection::{self, StreamCommand}, + connection::{self, Rtt, StreamCommand}, frame::{ header::{Data, Header, StreamId, WindowUpdate}, Frame, @@ -26,6 +26,7 @@ use futures::{ }; use parking_lot::{Mutex, MutexGuard}; use std::convert::TryInto; +use std::time::Instant; use std::{ fmt, io, pin::Pin, @@ -118,6 +119,7 @@ impl Stream { config: Arc, credit: u32, sender: mpsc::Sender, + rtt: Rtt, ) -> Self { Self { id, @@ -125,7 +127,7 @@ impl Stream { config: config.clone(), sender, flag: Flag::None, - shared: Arc::new(Mutex::new(Shared::new(DEFAULT_CREDIT, credit, config))), + shared: Arc::new(Mutex::new(Shared::new(DEFAULT_CREDIT, credit, config, rtt))), } } @@ -135,6 +137,7 @@ impl Stream { config: Arc, window: u32, sender: mpsc::Sender, + rtt: Rtt, ) -> Self { Self { id, @@ -142,7 +145,7 @@ impl Stream { config: config.clone(), sender, flag: Flag::None, - shared: Arc::new(Mutex::new(Shared::new(window, DEFAULT_CREDIT, config))), + shared: Arc::new(Mutex::new(Shared::new(window, DEFAULT_CREDIT, config, rtt))), } } @@ -419,25 +422,32 @@ impl AsyncWrite for Stream { pub(crate) struct Shared { state: State, pub(crate) window: u32, + pub(crate) window_max: u32, pub(crate) credit: u32, pub(crate) buffer: Chunks, pub(crate) reader: Option, pub(crate) writer: Option, + pub(crate) last_window_update: Instant, config: Arc, + // TODO: Doesn't make sense to have this behind a mutex (shared) again. + rtt: Rtt, } impl Shared { - fn new(window: u32, credit: u32, config: Arc) -> Self { + fn new(window: u32, credit: u32, config: Arc, rtt: Rtt) -> Self { Shared { state: State::Open { acknowledged: false, }, window, + window_max: config.receive_window, credit, buffer: Chunks::new(), reader: None, writer: None, + last_window_update: Instant::now(), config, + rtt, } } @@ -494,24 +504,30 @@ impl Shared { return None; } - let new_credit = { - debug_assert!(self.config.receive_window >= self.window); - let bytes_received = self.config.receive_window.saturating_sub(self.window); - let buffer_len: u32 = self.buffer.len().try_into().unwrap_or(std::u32::MAX); + debug_assert!(self.window_max >= self.window); + let bytes_received = self.window_max.saturating_sub(self.window); + let buffer_len: u32 = self.buffer.len().try_into().unwrap_or(std::u32::MAX); + let mut new_credit = bytes_received.saturating_sub(buffer_len); - bytes_received.saturating_sub(buffer_len) - }; + if new_credit < self.window_max / 2 { + return None; + } - // Send WindowUpdate message when half or more of the configured receive - // window can be granted as additional credit to the sender. - // - // See https://github.com/paritytech/yamux/issues/100 for a detailed - // discussion. - if new_credit >= self.config.receive_window / 2 { - Some(new_credit) - } else { - None + if let Some(rtt) = self.rtt.rtt() { + if self.last_window_update.elapsed() < rtt * 2 { + // TODO: Bound by stream limit and connection limit. + // E.g. take into account that connection_limit - (max_streams * 256KB) - what_is_already_taken + self.window_max = self.window_max.saturating_mul(2); + log::debug!("new window_max: {}", self.window_max); + + let bytes_received = self.window_max.saturating_sub(self.window); + let buffer_len: u32 = self.buffer.len().try_into().unwrap_or(std::u32::MAX); + new_credit = bytes_received.saturating_sub(buffer_len); + } } + + self.last_window_update = Instant::now(); + return Some(new_credit); } /// Whether we are still waiting for the remote to acknowledge this stream. diff --git a/yamux/src/frame.rs b/yamux/src/frame.rs index 692840a4..be010a3b 100644 --- a/yamux/src/frame.rs +++ b/yamux/src/frame.rs @@ -132,6 +132,22 @@ impl Frame { } } +impl Frame { + pub fn ping(nonce: u32) -> Self { + let mut header = Header::ping(nonce); + header.syn(); + + Frame { + header, + body: Vec::new(), + } + } + + pub fn nonce(&self) -> u32 { + self.header.nonce() + } +} + impl Frame { pub fn term() -> Self { Frame { diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index 1df9d193..bb1be077 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -85,8 +85,9 @@ pub struct Config { impl Default for Config { fn default() -> Self { Config { - receive_window: DEFAULT_CREDIT, - max_buffer_size: 1024 * 1024, + receive_window: 16 * 1024 * 1024, + max_buffer_size: 16*1024*1024, + // TODO max_num_streams: 8192, read_after_close: true, split_send_size: DEFAULT_SPLIT_SEND_SIZE, From c6ca2e52ca8ebd21e6120746c9e13a6ed5daa4c6 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 22 Nov 2023 19:12:44 +0100 Subject: [PATCH 02/39] expose rtt --- yamux/Cargo.toml | 1 - yamux/src/connection.rs | 4 ++-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/yamux/Cargo.toml b/yamux/Cargo.toml index 103a99c9..e919d679 100644 --- a/yamux/Cargo.toml +++ b/yamux/Cargo.toml @@ -11,7 +11,6 @@ edition = "2021" [dependencies] futures = { version = "0.3.12", default-features = false, features = ["std", "executor"] } -futures-timer = "3" log = "0.4.8" nohash-hasher = "0.2" parking_lot = "0.12" diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 5e6905ec..186c7504 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -293,10 +293,10 @@ struct Active { } #[derive(Clone, Debug)] -struct Rtt(Arc>); +pub struct Rtt(Arc>); impl Rtt { - pub fn new() -> Self { + fn new() -> Self { Self(Arc::new(Mutex::new(RttInner { rtt: None, state: RttState::Initial, From b8764d27bbe0cd2f3b65e3906f21b068d3d81b17 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 23 Nov 2023 10:59:05 +0100 Subject: [PATCH 03/39] Introduce connection limit --- test-harness/src/lib.rs | 2 +- test-harness/tests/poll_api.rs | 8 ++- yamux/src/connection.rs | 65 ++++++++++++++++------- yamux/src/connection/stream.rs | 96 +++++++++++++++++++++++++++------- yamux/src/frame.rs | 6 +-- yamux/src/lib.rs | 33 +++++++++--- 6 files changed, 159 insertions(+), 51 deletions(-) diff --git a/test-harness/src/lib.rs b/test-harness/src/lib.rs index e02c12e0..72e2a596 100644 --- a/test-harness/src/lib.rs +++ b/test-harness/src/lib.rs @@ -449,7 +449,7 @@ impl Arbitrary for TestConfig { fn arbitrary(g: &mut Gen) -> Self { let mut c = Config::default(); c.set_read_after_close(Arbitrary::arbitrary(g)); - c.set_receive_window(256 * 1024 + u32::arbitrary(g) % (768 * 1024)); + c.set_receive_window(256 * 1024 + usize::arbitrary(g) % (768 * 1024)); TestConfig(c) } } diff --git a/test-harness/tests/poll_api.rs b/test-harness/tests/poll_api.rs index 4d21496e..164952f6 100644 --- a/test-harness/tests/poll_api.rs +++ b/test-harness/tests/poll_api.rs @@ -12,7 +12,7 @@ use tokio::net::TcpStream; use tokio::runtime::Runtime; use tokio::task; use tokio_util::compat::TokioAsyncReadCompatExt; -use yamux::{Config, Connection, ConnectionError, Mode}; +use yamux::{Config, Connection, ConnectionError, Mode, DEFAULT_CREDIT}; #[test] fn prop_config_send_recv_multi() { @@ -62,6 +62,12 @@ fn concurrent_streams() { let mut cfg = Config::default(); cfg.set_split_send_size(PAYLOAD_SIZE); // Use a large frame size to speed up the test. + println!("here"); + + // TODO: Rethink these. + cfg.set_connection_window(n_streams * DEFAULT_CREDIT); + cfg.set_max_num_streams(n_streams); + Runtime::new().expect("new runtime").block_on(async move { let (server, client) = connected_peers(cfg.clone(), cfg, tcp_buffer_sizes) .await diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 186c7504..b33ede5c 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -290,6 +290,8 @@ struct Active { new_outbound_stream_waker: Option, rtt: Rtt, + + accumulated_max_stream_windows: Arc>, } #[derive(Clone, Debug)] @@ -432,6 +434,7 @@ impl Active { pending_frames: VecDeque::default(), new_outbound_stream_waker: None, rtt: Rtt::new(), + accumulated_max_stream_windows: Default::default(), } } @@ -520,20 +523,24 @@ impl Active { log::trace!("{}: creating new outbound stream", self.id); let id = self.next_stream_id()?; - let extra_credit = self.config.receive_window - DEFAULT_CREDIT; - - if extra_credit > 0 { - let mut frame = Frame::window_update(id, extra_credit); - frame.header_mut().syn(); - log::trace!("{}/{}: sending initial {}", self.id, id, frame.header()); - self.pending_frames.push_back(frame.into()); - } + // TODO: I don't think we want this with a dynamic receive window! + // + // let extra_credit = self.config.receive_window - DEFAULT_CREDIT; + // + // if extra_credit > 0 { + // let mut frame = Frame::window_update(id, extra_credit); + // frame.header_mut().syn(); + // log::trace!("{}/{}: sending initial {}", self.id, id, frame.header()); + // self.pending_frames.push_back(frame.into()); + // } - let mut stream = self.make_new_outbound_stream(id, self.config.receive_window); + let mut stream = self.make_new_outbound_stream(id); - if extra_credit == 0 { - stream.set_flag(stream::Flag::Syn) - } + // TODO: I don't think we want this with a dynamic receive window! + // if extra_credit == 0 { + // stream.set_flag(stream::Flag::Syn) + // } + stream.set_flag(stream::Flag::Syn); log::debug!("{}: new outbound {} of {}", self.id, stream, self); self.streams.insert(id, stream.clone_shared()); @@ -734,9 +741,10 @@ impl Active { // TODO let max_buffer_size = shared.window_max as usize; if shared.buffer.len() >= max_buffer_size { - panic!( + log::error!( "{}/{}: buffer of stream grows beyond limit", - self.id, stream_id + self.id, + stream_id ); let mut header = Header::data(stream_id, 0); header.rst(); @@ -801,7 +809,8 @@ impl Active { return Action::Terminate(Frame::protocol_error()); } - let credit = frame.header().credit() + DEFAULT_CREDIT; + // TODO: Is the cast safe? + let credit = frame.header().credit() as usize + DEFAULT_CREDIT; let mut stream = self.make_new_inbound_stream(stream_id, credit); stream.set_flag(stream::Flag::Ack); @@ -816,7 +825,8 @@ impl Active { if let Some(s) = self.streams.get_mut(&stream_id) { let mut shared = s.lock(); - shared.credit += frame.header().credit(); + // TODO: Is the cast safe? + shared.credit += frame.header().credit() as usize; if is_finish { shared.update_state(self.id, stream_id, State::RecvClosed); } @@ -869,7 +879,7 @@ impl Active { Action::None } - fn make_new_inbound_stream(&mut self, id: StreamId, credit: u32) -> Stream { + fn make_new_inbound_stream(&mut self, id: StreamId, credit: usize) -> Stream { let config = self.config.clone(); let (sender, receiver) = mpsc::channel(10); // 10 is an arbitrary number. @@ -878,10 +888,18 @@ impl Active { waker.wake(); } - Stream::new_inbound(id, self.id, config, credit, sender, self.rtt.clone()) + Stream::new_inbound( + id, + self.id, + config, + credit, + sender, + self.rtt.clone(), + self.accumulated_max_stream_windows.clone(), + ) } - fn make_new_outbound_stream(&mut self, id: StreamId, window: u32) -> Stream { + fn make_new_outbound_stream(&mut self, id: StreamId) -> Stream { let config = self.config.clone(); let (sender, receiver) = mpsc::channel(10); // 10 is an arbitrary number. @@ -890,7 +908,14 @@ impl Active { waker.wake(); } - Stream::new_outbound(id, self.id, config, window, sender, self.rtt.clone()) + Stream::new_outbound( + id, + self.id, + config, + sender, + self.rtt.clone(), + self.accumulated_max_stream_windows.clone(), + ) } fn next_stream_id(&mut self) -> Result { diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 826285a4..2de81c4d 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -117,9 +117,10 @@ impl Stream { id: StreamId, conn: connection::Id, config: Arc, - credit: u32, + credit: usize, sender: mpsc::Sender, rtt: Rtt, + accumulated_max_stream_windows: Arc>, ) -> Self { Self { id, @@ -127,7 +128,13 @@ impl Stream { config: config.clone(), sender, flag: Flag::None, - shared: Arc::new(Mutex::new(Shared::new(DEFAULT_CREDIT, credit, config, rtt))), + shared: Arc::new(Mutex::new(Shared::new( + DEFAULT_CREDIT, + credit, + config, + rtt, + accumulated_max_stream_windows, + ))), } } @@ -135,9 +142,9 @@ impl Stream { id: StreamId, conn: connection::Id, config: Arc, - window: u32, sender: mpsc::Sender, rtt: Rtt, + accumulated_max_stream_windows: Arc>, ) -> Self { Self { id, @@ -145,7 +152,13 @@ impl Stream { config: config.clone(), sender, flag: Flag::None, - shared: Arc::new(Mutex::new(Shared::new(window, DEFAULT_CREDIT, config, rtt))), + shared: Arc::new(Mutex::new(Shared::new( + DEFAULT_CREDIT, + DEFAULT_CREDIT, + config, + rtt, + accumulated_max_stream_windows, + ))), } } @@ -214,7 +227,9 @@ impl Stream { shared.window += credit; drop(shared); - let mut frame = Frame::window_update(self.id, credit).right(); + // TODO: Handle the case where credit is larger than u32::MAX. Use the smaller and don't + // bump shared.window with the higher. + let mut frame = Frame::window_update(self.id, credit as u32).right(); self.add_flag(frame.header_mut()); let cmd = StreamCommand::SendFrame(frame); self.sender @@ -363,7 +378,7 @@ impl AsyncWrite for Stream { } let k = std::cmp::min(shared.credit as usize, buf.len()); let k = std::cmp::min(k, self.config.split_send_size); - shared.credit = shared.credit.saturating_sub(k as u32); + shared.credit = shared.credit.saturating_sub(k); Vec::from(&buf[..k]) }; let n = body.len(); @@ -421,9 +436,11 @@ impl AsyncWrite for Stream { #[derive(Debug)] pub(crate) struct Shared { state: State, - pub(crate) window: u32, - pub(crate) window_max: u32, - pub(crate) credit: u32, + // TODO: Rename this to inbound_window or receive_window? + pub(crate) window: usize, + pub(crate) window_max: usize, + // TODO: Rename this to outbound_window or better event send_window? + pub(crate) credit: usize, pub(crate) buffer: Chunks, pub(crate) reader: Option, pub(crate) writer: Option, @@ -431,16 +448,34 @@ pub(crate) struct Shared { config: Arc, // TODO: Doesn't make sense to have this behind a mutex (shared) again. rtt: Rtt, + // TODO: Doesn't make sense to have this behind a mutex (shared) again. + accumulated_max_stream_windows: Arc>, +} + +impl Drop for Shared { + fn drop(&mut self) { + *self.accumulated_max_stream_windows.lock() -= self.window_max - DEFAULT_CREDIT; + } } impl Shared { - fn new(window: u32, credit: u32, config: Arc, rtt: Rtt) -> Self { + fn new( + window: usize, + credit: usize, + config: Arc, + rtt: Rtt, + accumulated_max_stream_windows: Arc>, + ) -> Self { + let window_max = DEFAULT_CREDIT; + // TODO: We don't add it here. default credit is implied + // *accumulated_max_stream_windows.lock() = window_max; + Shared { state: State::Open { acknowledged: false, }, window, - window_max: config.receive_window, + window_max, credit, buffer: Chunks::new(), reader: None, @@ -448,6 +483,7 @@ impl Shared { last_window_update: Instant::now(), config, rtt, + accumulated_max_stream_windows, } } @@ -499,14 +535,19 @@ impl Shared { /// /// Note: Once a caller successfully sent a window update message, the /// locally tracked window size needs to be updated manually by the caller. - pub(crate) fn next_window_update(&mut self) -> Option { + pub(crate) fn next_window_update(&mut self) -> Option { if !self.state.can_read() { return None; } - debug_assert!(self.window_max >= self.window); + debug_assert!( + self.window_max >= self.window, + "window_max {} window {}", + self.window_max, + self.window + ); let bytes_received = self.window_max.saturating_sub(self.window); - let buffer_len: u32 = self.buffer.len().try_into().unwrap_or(std::u32::MAX); + let buffer_len = self.buffer.len(); let mut new_credit = bytes_received.saturating_sub(buffer_len); if new_credit < self.window_max / 2 { @@ -515,13 +556,30 @@ impl Shared { if let Some(rtt) = self.rtt.rtt() { if self.last_window_update.elapsed() < rtt * 2 { - // TODO: Bound by stream limit and connection limit. - // E.g. take into account that connection_limit - (max_streams * 256KB) - what_is_already_taken - self.window_max = self.window_max.saturating_mul(2); - log::debug!("new window_max: {}", self.window_max); + let previous_window_max = self.window_max; + + let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); + + self.window_max = std::cmp::min( + std::cmp::min( + self.window_max.saturating_mul(2), + self.config.receive_window, + ), + self.window_max + + ((self.config.connection_window + - self.config.max_num_streams * DEFAULT_CREDIT) + - *accumulated_max_stream_windows), + ); + + *accumulated_max_stream_windows += self.window_max - previous_window_max; + log::debug!( + "old/new window_max: {}/{}", + previous_window_max, + self.window_max + ); let bytes_received = self.window_max.saturating_sub(self.window); - let buffer_len: u32 = self.buffer.len().try_into().unwrap_or(std::u32::MAX); + let buffer_len = self.buffer.len(); new_credit = bytes_received.saturating_sub(buffer_len); } } diff --git a/yamux/src/frame.rs b/yamux/src/frame.rs index be010a3b..09291391 100644 --- a/yamux/src/frame.rs +++ b/yamux/src/frame.rs @@ -112,10 +112,8 @@ impl Frame { &self.body } - pub fn body_len(&self) -> u32 { - // Safe cast since we construct `Frame::`s only with - // `Vec` of length [0, u32::MAX] in `Frame::data` above. - self.body().len() as u32 + pub fn body_len(&self) -> usize { + self.body().len() } pub fn into_body(self) -> Vec { diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index bb1be077..78fa7c91 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -38,7 +38,7 @@ pub use crate::frame::{ FrameDecodeError, }; -pub const DEFAULT_CREDIT: u32 = 256 * 1024; // as per yamux specification +pub const DEFAULT_CREDIT: usize = 256 * 1024; // as per yamux specification pub type Result = std::result::Result; @@ -75,7 +75,10 @@ const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * 1024; /// - split send size = 16 KiB #[derive(Debug, Clone)] pub struct Config { - receive_window: u32, + // TODO: Rename to max_stream_receive_window + receive_window: usize, + // TODO: Rename to max_connection_receive_window + connection_window: usize, max_buffer_size: usize, max_num_streams: usize, read_after_close: bool, @@ -86,9 +89,12 @@ impl Default for Config { fn default() -> Self { Config { receive_window: 16 * 1024 * 1024, - max_buffer_size: 16*1024*1024, + // TODO: reevaluate default. + // TODO: Add setter. + connection_window: 1 * 1024 * 1024 * 1024, + max_buffer_size: 16 * 1024 * 1024, // TODO - max_num_streams: 8192, + max_num_streams: 512, read_after_close: true, split_send_size: DEFAULT_SPLIT_SEND_SIZE, } @@ -101,21 +107,29 @@ impl Config { /// # Panics /// /// If the given receive window is < 256 KiB. - pub fn set_receive_window(&mut self, n: u32) -> &mut Self { - assert!(n >= DEFAULT_CREDIT); + pub fn set_receive_window(&mut self, n: usize) -> &mut Self { self.receive_window = n; + self.check(); + self + } + + pub fn set_connection_window(&mut self, n: usize) -> &mut Self { + self.connection_window = n; + self.check(); self } /// Set the max. buffer size per stream. pub fn set_max_buffer_size(&mut self, n: usize) -> &mut Self { self.max_buffer_size = n; + self.check(); self } /// Set the max. number of streams. pub fn set_max_num_streams(&mut self, n: usize) -> &mut Self { self.max_num_streams = n; + self.check(); self } @@ -123,6 +137,7 @@ impl Config { /// the connection has been closed. pub fn set_read_after_close(&mut self, b: bool) -> &mut Self { self.read_after_close = b; + self.check(); self } @@ -130,8 +145,14 @@ impl Config { /// than the configured max. will be split. pub fn set_split_send_size(&mut self, n: usize) -> &mut Self { self.split_send_size = n; + self.check(); self } + + fn check(&self) { + assert!(self.receive_window >= DEFAULT_CREDIT); + assert!(self.connection_window >= self.max_num_streams * DEFAULT_CREDIT); + } } // Check that we can safely cast a `usize` to a `u64`. From 1839e8b44b3c53a0fb807b76c14e307c6b16f488 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 23 Nov 2023 12:13:56 +0100 Subject: [PATCH 04/39] improve logging --- test-harness/tests/poll_api.rs | 2 -- yamux/src/connection.rs | 5 ++++- yamux/src/connection/stream.rs | 14 +++++++++++--- 3 files changed, 15 insertions(+), 6 deletions(-) diff --git a/test-harness/tests/poll_api.rs b/test-harness/tests/poll_api.rs index 164952f6..1f7b1c76 100644 --- a/test-harness/tests/poll_api.rs +++ b/test-harness/tests/poll_api.rs @@ -62,8 +62,6 @@ fn concurrent_streams() { let mut cfg = Config::default(); cfg.set_split_send_size(PAYLOAD_SIZE); // Use a large frame size to speed up the test. - println!("here"); - // TODO: Rethink these. cfg.set_connection_window(n_streams * DEFAULT_CREDIT); cfg.set_max_num_streams(n_streams); diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index b33ede5c..94f9c1a2 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -338,7 +338,10 @@ impl Rtt { } inner.rtt = Some(sent_at.elapsed()); - println!("{}", inner.rtt.as_ref().unwrap().as_secs_f64()); + log::debug!( + "estimated round-trip-time: {}s", + inner.rtt.as_ref().unwrap().as_secs_f64() + ); // TODO inner.state = RttState::Waiting { next: Instant::now() + Duration::from_secs(10), diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 2de81c4d..7a4893db 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -550,6 +550,14 @@ impl Shared { let buffer_len = self.buffer.len(); let mut new_credit = bytes_received.saturating_sub(buffer_len); + log::debug!( + "received {} mb in {} seconds ({} mbit/s)", + new_credit as f64 / 1024.0 / 1024.0, + self.last_window_update.elapsed().as_secs_f64(), + new_credit as f64 / 1024.0 / 1024.0 * 8.0 + / self.last_window_update.elapsed().as_secs_f64() + ); + if new_credit < self.window_max / 2 { return None; } @@ -573,9 +581,9 @@ impl Shared { *accumulated_max_stream_windows += self.window_max - previous_window_max; log::debug!( - "old/new window_max: {}/{}", - previous_window_max, - self.window_max + "old window_max: {} mb, new window_max: {} mb", + previous_window_max as f64 / 1024.0 / 1024.0, + self.window_max as f64 / 1024.0 / 1024.0 ); let bytes_received = self.window_max.saturating_sub(self.window); From 5dc40c2b7956fc3ef66cbac8f2a618190e46e0fe Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 23 Nov 2023 12:15:27 +0100 Subject: [PATCH 05/39] less logging --- yamux/src/connection/stream.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 7a4893db..0ccc638f 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -550,6 +550,10 @@ impl Shared { let buffer_len = self.buffer.len(); let mut new_credit = bytes_received.saturating_sub(buffer_len); + if new_credit < self.window_max / 2 { + return None; + } + log::debug!( "received {} mb in {} seconds ({} mbit/s)", new_credit as f64 / 1024.0 / 1024.0, @@ -558,10 +562,6 @@ impl Shared { / self.last_window_update.elapsed().as_secs_f64() ); - if new_credit < self.window_max / 2 { - return None; - } - if let Some(rtt) = self.rtt.rtt() { if self.last_window_update.elapsed() < rtt * 2 { let previous_window_max = self.window_max; From dd4cf3a454e9db09310dde6c0031f7acfac3ec21 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Thu, 23 Nov 2023 15:31:47 +0100 Subject: [PATCH 06/39] Unbounded stream receive window Depend on connection window only. --- test-harness/src/lib.rs | 6 +++++- yamux/src/connection/stream.rs | 4 ++-- yamux/src/lib.rs | 12 +++++++----- 3 files changed, 14 insertions(+), 8 deletions(-) diff --git a/test-harness/src/lib.rs b/test-harness/src/lib.rs index 72e2a596..acd43cc9 100644 --- a/test-harness/src/lib.rs +++ b/test-harness/src/lib.rs @@ -449,7 +449,11 @@ impl Arbitrary for TestConfig { fn arbitrary(g: &mut Gen) -> Self { let mut c = Config::default(); c.set_read_after_close(Arbitrary::arbitrary(g)); - c.set_receive_window(256 * 1024 + usize::arbitrary(g) % (768 * 1024)); + if bool::arbitrary(g) { + c.set_receive_window(Some(256 * 1024 + usize::arbitrary(g) % (768 * 1024))); + } else { + c.set_receive_window(None); + } TestConfig(c) } } diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 0ccc638f..08c754ac 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -25,7 +25,6 @@ use futures::{ ready, SinkExt, }; use parking_lot::{Mutex, MutexGuard}; -use std::convert::TryInto; use std::time::Instant; use std::{ fmt, io, @@ -546,6 +545,7 @@ impl Shared { self.window_max, self.window ); + let bytes_received = self.window_max.saturating_sub(self.window); let buffer_len = self.buffer.len(); let mut new_credit = bytes_received.saturating_sub(buffer_len); @@ -571,7 +571,7 @@ impl Shared { self.window_max = std::cmp::min( std::cmp::min( self.window_max.saturating_mul(2), - self.config.receive_window, + self.config.receive_window.unwrap_or(usize::MAX), ), self.window_max + ((self.config.connection_window diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index 78fa7c91..09534860 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -76,7 +76,7 @@ const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * 1024; #[derive(Debug, Clone)] pub struct Config { // TODO: Rename to max_stream_receive_window - receive_window: usize, + receive_window: Option, // TODO: Rename to max_connection_receive_window connection_window: usize, max_buffer_size: usize, @@ -88,7 +88,8 @@ pub struct Config { impl Default for Config { fn default() -> Self { Config { - receive_window: 16 * 1024 * 1024, + // TODO: Add rational: given that we have a connection window, ... + receive_window: None, // TODO: reevaluate default. // TODO: Add setter. connection_window: 1 * 1024 * 1024 * 1024, @@ -107,12 +108,12 @@ impl Config { /// # Panics /// /// If the given receive window is < 256 KiB. - pub fn set_receive_window(&mut self, n: usize) -> &mut Self { + pub fn set_receive_window(&mut self, n: Option) -> &mut Self { self.receive_window = n; self.check(); self } - + pub fn set_connection_window(&mut self, n: usize) -> &mut Self { self.connection_window = n; self.check(); @@ -149,8 +150,9 @@ impl Config { self } + // TODO: Consider doing the check on creation, not on each builder method call. fn check(&self) { - assert!(self.receive_window >= DEFAULT_CREDIT); + assert!(self.receive_window.unwrap_or(usize::MAX) >= DEFAULT_CREDIT); assert!(self.connection_window >= self.max_num_streams * DEFAULT_CREDIT); } } From 24ca8718136f6cae85be5d93564e89fde7379f2a Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 24 Nov 2023 10:50:41 +0100 Subject: [PATCH 07/39] Rename config options --- test-harness/src/lib.rs | 4 ++-- test-harness/tests/poll_api.rs | 2 +- yamux/src/connection/stream.rs | 4 ++-- yamux/src/lib.rs | 22 ++++++++++++---------- 4 files changed, 17 insertions(+), 15 deletions(-) diff --git a/test-harness/src/lib.rs b/test-harness/src/lib.rs index acd43cc9..41df5004 100644 --- a/test-harness/src/lib.rs +++ b/test-harness/src/lib.rs @@ -450,9 +450,9 @@ impl Arbitrary for TestConfig { let mut c = Config::default(); c.set_read_after_close(Arbitrary::arbitrary(g)); if bool::arbitrary(g) { - c.set_receive_window(Some(256 * 1024 + usize::arbitrary(g) % (768 * 1024))); + c.set_max_stream_receive_window(Some(256 * 1024 + usize::arbitrary(g) % (768 * 1024))); } else { - c.set_receive_window(None); + c.set_max_stream_receive_window(None); } TestConfig(c) } diff --git a/test-harness/tests/poll_api.rs b/test-harness/tests/poll_api.rs index 1f7b1c76..1011aeb6 100644 --- a/test-harness/tests/poll_api.rs +++ b/test-harness/tests/poll_api.rs @@ -63,7 +63,7 @@ fn concurrent_streams() { cfg.set_split_send_size(PAYLOAD_SIZE); // Use a large frame size to speed up the test. // TODO: Rethink these. - cfg.set_connection_window(n_streams * DEFAULT_CREDIT); + cfg.set_max_connection_receive_window(n_streams * DEFAULT_CREDIT); cfg.set_max_num_streams(n_streams); Runtime::new().expect("new runtime").block_on(async move { diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 08c754ac..4636e824 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -571,10 +571,10 @@ impl Shared { self.window_max = std::cmp::min( std::cmp::min( self.window_max.saturating_mul(2), - self.config.receive_window.unwrap_or(usize::MAX), + self.config.max_stream_receive_window.unwrap_or(usize::MAX), ), self.window_max - + ((self.config.connection_window + + ((self.config.max_connection_receive_window - self.config.max_num_streams * DEFAULT_CREDIT) - *accumulated_max_stream_windows), ); diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index 09534860..c95e41cc 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -63,6 +63,8 @@ const MAX_ACK_BACKLOG: usize = 256; /// https://github.com/paritytech/yamux/issues/100. const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * 1024; + +// TODO: Update /// Yamux configuration. /// /// The default configuration values are as follows: @@ -76,9 +78,9 @@ const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * 1024; #[derive(Debug, Clone)] pub struct Config { // TODO: Rename to max_stream_receive_window - receive_window: Option, + max_stream_receive_window: Option, // TODO: Rename to max_connection_receive_window - connection_window: usize, + max_connection_receive_window: usize, max_buffer_size: usize, max_num_streams: usize, read_after_close: bool, @@ -89,10 +91,10 @@ impl Default for Config { fn default() -> Self { Config { // TODO: Add rational: given that we have a connection window, ... - receive_window: None, + max_stream_receive_window: None, // TODO: reevaluate default. // TODO: Add setter. - connection_window: 1 * 1024 * 1024 * 1024, + max_connection_receive_window: 1 * 1024 * 1024 * 1024, max_buffer_size: 16 * 1024 * 1024, // TODO max_num_streams: 512, @@ -108,14 +110,14 @@ impl Config { /// # Panics /// /// If the given receive window is < 256 KiB. - pub fn set_receive_window(&mut self, n: Option) -> &mut Self { - self.receive_window = n; + pub fn set_max_stream_receive_window(&mut self, n: Option) -> &mut Self { + self.max_stream_receive_window = n; self.check(); self } - pub fn set_connection_window(&mut self, n: usize) -> &mut Self { - self.connection_window = n; + pub fn set_max_connection_receive_window(&mut self, n: usize) -> &mut Self { + self.max_connection_receive_window = n; self.check(); self } @@ -152,8 +154,8 @@ impl Config { // TODO: Consider doing the check on creation, not on each builder method call. fn check(&self) { - assert!(self.receive_window.unwrap_or(usize::MAX) >= DEFAULT_CREDIT); - assert!(self.connection_window >= self.max_num_streams * DEFAULT_CREDIT); + assert!(self.max_stream_receive_window.unwrap_or(usize::MAX) >= DEFAULT_CREDIT); + assert!(self.max_connection_receive_window >= self.max_num_streams * DEFAULT_CREDIT); } } From 5ee74d1fb67091a89176383eecb3423aeb4ba9b3 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 24 Nov 2023 13:31:06 +0100 Subject: [PATCH 08/39] Remove max_buffer_size and rename variables --- test-harness/src/lib.rs | 2 +- test-harness/tests/poll_api.rs | 4 +- yamux/src/connection.rs | 16 +- yamux/src/connection/stream.rs | 285 ++++++++++++++++++--------------- yamux/src/frame/io.rs | 10 +- yamux/src/lib.rs | 11 -- 6 files changed, 170 insertions(+), 158 deletions(-) diff --git a/test-harness/src/lib.rs b/test-harness/src/lib.rs index 41df5004..14509d6e 100644 --- a/test-harness/src/lib.rs +++ b/test-harness/src/lib.rs @@ -76,7 +76,7 @@ where .try_for_each_concurrent(None, |mut stream| async move { { let (mut r, mut w) = AsyncReadExt::split(&mut stream); - futures::io::copy(&mut r, &mut w).await?; + futures::io::copy(&mut r, &mut w).await.unwrap(); } stream.close().await?; Ok(()) diff --git a/test-harness/tests/poll_api.rs b/test-harness/tests/poll_api.rs index 1011aeb6..bb77ed0d 100644 --- a/test-harness/tests/poll_api.rs +++ b/test-harness/tests/poll_api.rs @@ -177,7 +177,7 @@ fn prop_config_send_recv_single() { msgs.insert(0, Msg(vec![1u8; yamux::DEFAULT_CREDIT as usize])); Runtime::new().unwrap().block_on(async move { - let (server, mut client) = connected_peers(cfg1, cfg2, None).await?; + let (server, mut client) = connected_peers(cfg1, cfg2, None).await.unwrap(); let server = echo_server(server); let client = async { @@ -193,7 +193,7 @@ fn prop_config_send_recv_single() { Ok(()) }; - futures::future::try_join(server, client).await?; + futures::future::try_join(server, client).await.unwrap(); Ok(()) }) diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 94f9c1a2..8198e55c 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -421,7 +421,7 @@ impl Active { fn new(socket: T, cfg: Config, mode: Mode) -> Self { let id = Id::random(); log::debug!("new connection: {} ({:?})", id, mode); - let socket = frame::Io::new(id, socket, cfg.max_buffer_size).fuse(); + let socket = frame::Io::new(id, socket).fuse(); Active { id, mode, @@ -720,7 +720,9 @@ impl Active { if is_finish { shared.update_state(self.id, stream_id, State::RecvClosed); } - shared.window = shared.window.saturating_sub(frame.body_len()); + shared.current_receive_window_size = shared + .current_receive_window_size + .saturating_sub(frame.body_len()); shared.buffer.push(frame.into_body()); } stream.set_flag(stream::Flag::Ack); @@ -730,7 +732,7 @@ impl Active { if let Some(s) = self.streams.get_mut(&stream_id) { let mut shared = s.lock(); - if frame.body().len() > shared.window as usize { + if frame.body().len() > shared.current_receive_window_size as usize { log::error!( "{}/{}: frame body larger than window of stream", self.id, @@ -742,7 +744,7 @@ impl Active { shared.update_state(self.id, stream_id, State::RecvClosed); } // TODO - let max_buffer_size = shared.window_max as usize; + let max_buffer_size = shared.max_receive_window_size as usize; if shared.buffer.len() >= max_buffer_size { log::error!( "{}/{}: buffer of stream grows beyond limit", @@ -753,7 +755,9 @@ impl Active { header.rst(); return Action::Reset(Frame::new(header)); } - shared.window = shared.window.saturating_sub(frame.body_len()); + shared.current_receive_window_size = shared + .current_receive_window_size + .saturating_sub(frame.body_len()); shared.buffer.push(frame.into_body()); if let Some(w) = shared.reader.take() { w.wake() @@ -829,7 +833,7 @@ impl Active { if let Some(s) = self.streams.get_mut(&stream_id) { let mut shared = s.lock(); // TODO: Is the cast safe? - shared.credit += frame.header().credit() as usize; + shared.current_send_window_size += frame.header().credit() as usize; if is_finish { shared.update_state(self.id, stream_id, State::RecvClosed); } diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 4636e824..6eb7b3a9 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -25,6 +25,7 @@ use futures::{ ready, SinkExt, }; use parking_lot::{Mutex, MutexGuard}; +use std::cmp; use std::time::Instant; use std::{ fmt, io, @@ -94,6 +95,9 @@ pub struct Stream { sender: mpsc::Sender, flag: Flag, shared: Arc>, + accumulated_max_stream_windows: Arc>, + rtt: Rtt, + last_window_update: Instant, } impl fmt::Debug for Stream { @@ -127,13 +131,10 @@ impl Stream { config: config.clone(), sender, flag: Flag::None, - shared: Arc::new(Mutex::new(Shared::new( - DEFAULT_CREDIT, - credit, - config, - rtt, - accumulated_max_stream_windows, - ))), + shared: Arc::new(Mutex::new(Shared::new(DEFAULT_CREDIT, credit, config))), + accumulated_max_stream_windows, + rtt, + last_window_update: Instant::now(), } } @@ -155,9 +156,10 @@ impl Stream { DEFAULT_CREDIT, DEFAULT_CREDIT, config, - rtt, - accumulated_max_stream_windows, ))), + accumulated_max_stream_windows, + rtt, + last_window_update: Instant::now(), } } @@ -215,28 +217,134 @@ impl Stream { /// Send new credit to the sending side via a window update message if /// permitted. fn send_window_update(&mut self, cx: &mut Context) -> Poll> { + if !self.shared.lock().state.can_read() { + return Poll::Ready(Ok(())); + } + + ready!(self + .sender + .poll_ready(cx) + .map_err(|_| self.write_zero_err())?); + + let Some(credit) = self.next_window_update() else { + return Poll::Ready(Ok(())); + }; + + // TODO: Handle the case where credit is larger than u32::MAX. Use the smaller and don't + // bump shared.window with the higher. + let mut frame = Frame::window_update(self.id, credit as u32).right(); + self.add_flag(frame.header_mut()); + let cmd = StreamCommand::SendFrame(frame); + self.sender + .start_send(cmd) + .map_err(|_| self.write_zero_err())?; + + Poll::Ready(Ok(())) + } + + /// Calculate the number of additional window bytes the receiving side (local) should grant the + /// sending side (remote) via a window update message. + /// + /// Returns `None` if too small to justify a window update message. + fn next_window_update(&mut self) -> Option { let mut shared = self.shared.lock(); - if let Some(credit) = shared.next_window_update() { - ready!(self - .sender - .poll_ready(cx) - .map_err(|_| self.write_zero_err())?); - - shared.window += credit; - drop(shared); - - // TODO: Handle the case where credit is larger than u32::MAX. Use the smaller and don't - // bump shared.window with the higher. - let mut frame = Frame::window_update(self.id, credit as u32).right(); - self.add_flag(frame.header_mut()); - let cmd = StreamCommand::SendFrame(frame); - self.sender - .start_send(cmd) - .map_err(|_| self.write_zero_err())?; + debug_assert!( + shared.max_receive_window_size >= shared.current_receive_window_size, + "max_receive_window_size: {} current_receive_window_size: {}", + shared.max_receive_window_size, + shared.current_receive_window_size + ); + + let bytes_received = shared.max_receive_window_size - shared.current_receive_window_size; + let mut next_window_update = bytes_received.saturating_sub(shared.buffer.len()); + + // Don't send an update in case half or more of the window is still available to the sender. + if next_window_update < shared.max_receive_window_size / 2 { + return None; } - Poll::Ready(Ok(())) + log::debug!( + "received {} mb in {} seconds ({} mbit/s)", + next_window_update as f64 / 1024.0 / 1024.0, + self.last_window_update.elapsed().as_secs_f64(), + next_window_update as f64 / 1024.0 / 1024.0 * 8.0 + / self.last_window_update.elapsed().as_secs_f64() + ); + + // Auto-tuning `max_receive_window_size` + // + // The ideal `max_receive_window_size` is equal to the bandwidth-delay-product (BDP), thus + // allowing the remote sender to exhaust the entire available bandwidth on a single stream. + // Choosing `max_receive_window_size` too small prevents the remote sender from exhausting + // the available bandwidth. Choosing `max_receive_window_size` to large is wasteful and + // delays backpressure from the receiver to the sender on the stream. + // + // In case the remote sender has exhausted half or more of its credit in less than 2 + // round-trips, try to double `max_receive_window_size`. + // + // For simplicity `max_receive_window_size` is never decreased. + // + // This implementation is heavily influenced by QUIC. See document below for rational on the + // above strategy. + // + // https://docs.google.com/document/d/1F2YfdDXKpy20WVKJueEf4abn_LVZHhMUMS5gX6Pgjl4/edit?usp=sharing + if self + .rtt + .rtt() + .map(|rtt| self.last_window_update.elapsed() < rtt * 2) + .unwrap_or(false) + { + let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); + + // Ideally one can just double it: + let mut new_max = shared.max_receive_window_size.saturating_mul(2); + + // Then one has to consider the configured stream limit: + new_max = cmp::min( + new_max, + shared + .config + .max_stream_receive_window + .unwrap_or(usize::MAX), + ); + + // Then one has to consider the configured connection limit: + new_max = { + let connection_limit = shared.max_receive_window_size + + // the overall configured conneciton limit + shared.config.max_connection_receive_window + // minus the minimum amount of window guaranteed to each stream + - shared.config.max_num_streams * DEFAULT_CREDIT + // minus the amount of bytes beyond the minimum amount (`DEFAULT_CREDIT`) + // already allocated by this and other streams on the connection. + - *accumulated_max_stream_windows; + + cmp::min(new_max, connection_limit) + }; + + // Account for the additional credit on the accumulated connection counter. + *accumulated_max_stream_windows += new_max - shared.max_receive_window_size; + drop(accumulated_max_stream_windows); + + log::debug!( + "old window_max: {} mb, new window_max: {} mb", + shared.max_receive_window_size as f64 / 1024.0 / 1024.0, + new_max as f64 / 1024.0 / 1024.0 + ); + + shared.max_receive_window_size = new_max; + + // Recalculate `next_window_update` with the new `max_receive_window_size`. + let bytes_received = + shared.max_receive_window_size - shared.current_receive_window_size; + next_window_update = bytes_received.saturating_sub(shared.buffer.len()); + } + + self.last_window_update = Instant::now(); + shared.current_receive_window_size += next_window_update; + + return Some(next_window_update); } } @@ -370,14 +478,14 @@ impl AsyncWrite for Stream { log::debug!("{}/{}: can no longer write", self.conn, self.id); return Poll::Ready(Err(self.write_zero_err())); } - if shared.credit == 0 { + if shared.current_send_window_size == 0 { log::trace!("{}/{}: no more credit left", self.conn, self.id); shared.writer = Some(cx.waker().clone()); return Poll::Pending; } - let k = std::cmp::min(shared.credit as usize, buf.len()); + let k = std::cmp::min(shared.current_send_window_size as usize, buf.len()); let k = std::cmp::min(k, self.config.split_send_size); - shared.credit = shared.credit.saturating_sub(k); + shared.current_send_window_size = shared.current_send_window_size.saturating_sub(k); Vec::from(&buf[..k]) }; let n = body.len(); @@ -432,57 +540,38 @@ impl AsyncWrite for Stream { } } +impl Drop for Stream { + fn drop(&mut self) { + *self.accumulated_max_stream_windows.lock() -= + self.shared.lock().max_receive_window_size - DEFAULT_CREDIT; + } +} + #[derive(Debug)] pub(crate) struct Shared { state: State, - // TODO: Rename this to inbound_window or receive_window? - pub(crate) window: usize, - pub(crate) window_max: usize, - // TODO: Rename this to outbound_window or better event send_window? - pub(crate) credit: usize, + pub(crate) current_receive_window_size: usize, + pub(crate) max_receive_window_size: usize, + pub(crate) current_send_window_size: usize, pub(crate) buffer: Chunks, pub(crate) reader: Option, pub(crate) writer: Option, - pub(crate) last_window_update: Instant, config: Arc, - // TODO: Doesn't make sense to have this behind a mutex (shared) again. - rtt: Rtt, - // TODO: Doesn't make sense to have this behind a mutex (shared) again. - accumulated_max_stream_windows: Arc>, -} - -impl Drop for Shared { - fn drop(&mut self) { - *self.accumulated_max_stream_windows.lock() -= self.window_max - DEFAULT_CREDIT; - } } impl Shared { - fn new( - window: usize, - credit: usize, - config: Arc, - rtt: Rtt, - accumulated_max_stream_windows: Arc>, - ) -> Self { - let window_max = DEFAULT_CREDIT; - // TODO: We don't add it here. default credit is implied - // *accumulated_max_stream_windows.lock() = window_max; - + fn new(window: usize, credit: usize, config: Arc) -> Self { Shared { state: State::Open { acknowledged: false, }, - window, - window_max, - credit, + current_receive_window_size: window, + max_receive_window_size: DEFAULT_CREDIT, + current_send_window_size: credit, buffer: Chunks::new(), reader: None, writer: None, - last_window_update: Instant::now(), config, - rtt, - accumulated_max_stream_windows, } } @@ -526,76 +615,6 @@ impl Shared { current // Return the previous stream state for informational purposes. } - // TODO: This does not need to live in shared any longer. - /// Calculate the number of additional window bytes the receiving side - /// should grant the sending side via a window update message. - /// - /// Returns `None` if too small to justify a window update message. - /// - /// Note: Once a caller successfully sent a window update message, the - /// locally tracked window size needs to be updated manually by the caller. - pub(crate) fn next_window_update(&mut self) -> Option { - if !self.state.can_read() { - return None; - } - - debug_assert!( - self.window_max >= self.window, - "window_max {} window {}", - self.window_max, - self.window - ); - - let bytes_received = self.window_max.saturating_sub(self.window); - let buffer_len = self.buffer.len(); - let mut new_credit = bytes_received.saturating_sub(buffer_len); - - if new_credit < self.window_max / 2 { - return None; - } - - log::debug!( - "received {} mb in {} seconds ({} mbit/s)", - new_credit as f64 / 1024.0 / 1024.0, - self.last_window_update.elapsed().as_secs_f64(), - new_credit as f64 / 1024.0 / 1024.0 * 8.0 - / self.last_window_update.elapsed().as_secs_f64() - ); - - if let Some(rtt) = self.rtt.rtt() { - if self.last_window_update.elapsed() < rtt * 2 { - let previous_window_max = self.window_max; - - let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); - - self.window_max = std::cmp::min( - std::cmp::min( - self.window_max.saturating_mul(2), - self.config.max_stream_receive_window.unwrap_or(usize::MAX), - ), - self.window_max - + ((self.config.max_connection_receive_window - - self.config.max_num_streams * DEFAULT_CREDIT) - - *accumulated_max_stream_windows), - ); - - *accumulated_max_stream_windows += self.window_max - previous_window_max; - log::debug!( - "old window_max: {} mb, new window_max: {} mb", - previous_window_max as f64 / 1024.0 / 1024.0, - self.window_max as f64 / 1024.0 / 1024.0 - ); - - let bytes_received = self.window_max.saturating_sub(self.window); - let buffer_len = self.buffer.len(); - new_credit = bytes_received.saturating_sub(buffer_len); - } - } - - self.last_window_update = Instant::now(); - return Some(new_credit); - } - /// Whether we are still waiting for the remote to acknowledge this stream. pub fn is_pending_ack(&self) -> bool { matches!( diff --git a/yamux/src/frame/io.rs b/yamux/src/frame/io.rs index 43e03987..b2a27b0e 100644 --- a/yamux/src/frame/io.rs +++ b/yamux/src/frame/io.rs @@ -20,6 +20,8 @@ use std::{ task::{Context, Poll}, }; +const MAX_FRAME_BODY_LEN: usize = 1024 * 1024; + /// A [`Stream`] and writer of [`Frame`] values. #[derive(Debug)] pub(crate) struct Io { @@ -27,17 +29,15 @@ pub(crate) struct Io { io: T, read_state: ReadState, write_state: WriteState, - max_body_len: usize, } impl Io { - pub(crate) fn new(id: Id, io: T, max_frame_body_len: usize) -> Self { + pub(crate) fn new(id: Id, io: T) -> Self { Io { id, io, read_state: ReadState::Init, write_state: WriteState::Init, - max_body_len: max_frame_body_len, } } } @@ -200,7 +200,7 @@ impl Stream for Io { let body_len = header.len().val() as usize; - if body_len > this.max_body_len { + if body_len > MAX_FRAME_BODY_LEN { return Poll::Ready(Some(Err(FrameDecodeError::FrameTooLarge( body_len, )))); @@ -349,7 +349,7 @@ mod tests { fn property(f: Frame<()>) -> bool { futures::executor::block_on(async move { let id = crate::connection::Id::random(); - let mut io = Io::new(id, futures::io::Cursor::new(Vec::new()), f.body.len()); + let mut io = Io::new(id, futures::io::Cursor::new(Vec::new())); if io.send(f.clone()).await.is_err() { return false; } diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index c95e41cc..c2e97fb0 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -77,11 +77,8 @@ const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * 1024; /// - split send size = 16 KiB #[derive(Debug, Clone)] pub struct Config { - // TODO: Rename to max_stream_receive_window max_stream_receive_window: Option, - // TODO: Rename to max_connection_receive_window max_connection_receive_window: usize, - max_buffer_size: usize, max_num_streams: usize, read_after_close: bool, split_send_size: usize, @@ -95,7 +92,6 @@ impl Default for Config { // TODO: reevaluate default. // TODO: Add setter. max_connection_receive_window: 1 * 1024 * 1024 * 1024, - max_buffer_size: 16 * 1024 * 1024, // TODO max_num_streams: 512, read_after_close: true, @@ -122,13 +118,6 @@ impl Config { self } - /// Set the max. buffer size per stream. - pub fn set_max_buffer_size(&mut self, n: usize) -> &mut Self { - self.max_buffer_size = n; - self.check(); - self - } - /// Set the max. number of streams. pub fn set_max_num_streams(&mut self, n: usize) -> &mut Self { self.max_num_streams = n; From 504e876a90892ac42d037e33adcf3620b3617f50 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 24 Nov 2023 13:34:58 +0100 Subject: [PATCH 09/39] Revert benchmark changes --- test-harness/benches/concurrent.rs | 23 +++++++++++++++-------- 1 file changed, 15 insertions(+), 8 deletions(-) diff --git a/test-harness/benches/concurrent.rs b/test-harness/benches/concurrent.rs index b204764a..e619de40 100644 --- a/test-harness/benches/concurrent.rs +++ b/test-harness/benches/concurrent.rs @@ -8,9 +8,9 @@ // at https://www.apache.org/licenses/LICENSE-2.0 and a copy of the MIT license // at https://opensource.org/licenses/MIT. -use constrained_connection::{new_unconstrained_connection, samples, Endpoint, new_constrained_connection}; +use constrained_connection::{new_unconstrained_connection, samples, Endpoint}; use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput}; -use std::{iter, time::Duration}; +use std::iter; use std::sync::Arc; use test_harness::{dev_null_server, MessageSender, MessageSenderStrategy, Msg}; use tokio::{runtime::Runtime, task}; @@ -29,19 +29,26 @@ impl AsRef<[u8]> for Bytes { } fn concurrent(c: &mut Criterion) { - let _ = env_logger::try_init(); - - let data = Bytes(Arc::new(vec![0x42; 10*1024*1024*1024])); + let data = Bytes(Arc::new(vec![0x42; 4096])); let networks = vec![ - ("aws-east-west", (|| new_constrained_connection(10*1000*1000*1000, Duration::from_millis(60))) as fn() -> (_, _)), + ("mobile", (|| samples::mobile_hsdpa().2) as fn() -> (_, _)), + ( + "adsl2+", + (|| samples::residential_adsl2().2) as fn() -> (_, _), + ), + ("gbit-lan", (|| samples::gbit_lan().2) as fn() -> (_, _)), + ( + "unconstrained", + new_unconstrained_connection as fn() -> (_, _), + ), ]; let mut group = c.benchmark_group("concurrent"); group.sample_size(10); for (network_name, new_connection) in networks.into_iter() { - for nstreams in [1].iter() { - for nmessages in [1].iter() { + for nstreams in [1, 10, 100].iter() { + for nmessages in [1, 10, 100].iter() { let data = data.clone(); let rt = Runtime::new().unwrap(); From 3e9a870277c7308c7cb94d5b525f13e1e7273cff Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 24 Nov 2023 13:56:47 +0100 Subject: [PATCH 10/39] Refactor rtt --- yamux/src/connection.rs | 57 ++++++++++++++++++++++++++--------------- 1 file changed, 36 insertions(+), 21 deletions(-) diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 8198e55c..ff44970c 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -307,11 +307,11 @@ impl Rtt { fn next_ping(&mut self) -> Option> { let state = &mut self.0.lock().state; + match state { RttState::Initial => {} RttState::AwaitingPong { .. } => return None, RttState::Waiting { next } => { - // TODO: Might be expensive in the hot path. Maybe don't put at the beginning of the overall poll? if *next > Instant::now() { return None; } @@ -324,31 +324,44 @@ impl Rtt { sent_at: Instant::now(), nonce, }; + + log::debug!("sending ping {nonce}"); // TODO: randomize nonce. Some(Frame::ping(nonce)) } - fn received_pong(&mut self, nonce: u32) { + fn handle_pong(&mut self, received_nonce: u32) -> Action { let inner = &mut self.0.lock(); - match inner.state { - RttState::Initial => todo!(), - RttState::AwaitingPong { sent_at, nonce: n } => { - if nonce != n { - todo!() - } - inner.rtt = Some(sent_at.elapsed()); - log::debug!( - "estimated round-trip-time: {}s", - inner.rtt.as_ref().unwrap().as_secs_f64() - ); - // TODO - inner.state = RttState::Waiting { - next: Instant::now() + Duration::from_secs(10), - }; + let (sent_at, expected_nonce) = match inner.state { + RttState::Initial | RttState::Waiting { .. } => { + log::error!("received unexpected pong {}", received_nonce); + return Action::Terminate(Frame::protocol_error()); } - RttState::Waiting { next } => todo!(), + RttState::AwaitingPong { sent_at, nonce } => (sent_at, nonce), + }; + + if received_nonce != expected_nonce { + log::error!( + "received pong with {} but expected {}", + received_nonce, + expected_nonce + ); + return Action::Terminate(Frame::protocol_error()); } + + inner.rtt = Some(sent_at.elapsed()); + log::debug!( + "received pong {received_nonce}, estimated round-trip-time {}s", + inner.rtt.as_ref().unwrap().as_secs_f64() + ); + + // TODO + inner.state = RttState::Waiting { + next: Instant::now() + Duration::from_secs(10), + }; + + return Action::None; } fn rtt(&self) -> Option { @@ -458,11 +471,14 @@ impl Active { fn poll(&mut self, cx: &mut Context<'_>) -> Poll> { loop { if self.socket.poll_ready_unpin(cx).is_ready() { + // TODO: Is this too expensive for it to be at the start of the loop? Could move it + // to the end, but a connection constantly writing and reading would then never send + // a ping. Also might be worth sending ping up front for better latency calc? if let Some(frame) = self.rtt.next_ping() { - log::debug!("sending ping"); self.socket.start_send_unpin(frame.into())?; continue; } + if let Some(frame) = self.pending_frames.pop_front() { self.socket.start_send_unpin(frame)?; continue; @@ -862,8 +878,7 @@ impl Active { fn on_ping(&mut self, frame: &Frame) -> Action { let stream_id = frame.header().stream_id(); if frame.header().flags().contains(header::ACK) { - self.rtt.received_pong(frame.nonce()); - return Action::None; + return self.rtt.handle_pong(frame.nonce()); } if stream_id == CONNECTION_ID || self.streams.contains_key(&stream_id) { let mut hdr = Header::ping(frame.header().nonce()); From 8de55cfb2976b027be8b2a4ceb81b372ad4b3eb3 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 24 Nov 2023 14:26:00 +0100 Subject: [PATCH 11/39] randomize nonce --- yamux/src/connection.rs | 28 +++++----------------------- yamux/src/connection/stream.rs | 2 +- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index ff44970c..3994c02f 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -37,6 +37,8 @@ use std::{fmt, sync::Arc, task::Poll}; pub use stream::{Packet, State, Stream}; +const PING_INTERVAL: Duration = Duration::from_secs(10); + /// How the connection is used. #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] pub enum Mode { @@ -318,15 +320,12 @@ impl Rtt { } } - let nonce = 42; - + let nonce = rand::random(); *state = RttState::AwaitingPong { sent_at: Instant::now(), nonce, }; - log::debug!("sending ping {nonce}"); - // TODO: randomize nonce. Some(Frame::ping(nonce)) } @@ -356,15 +355,14 @@ impl Rtt { inner.rtt.as_ref().unwrap().as_secs_f64() ); - // TODO inner.state = RttState::Waiting { - next: Instant::now() + Duration::from_secs(10), + next: Instant::now() + PING_INTERVAL, }; return Action::None; } - fn rtt(&self) -> Option { + fn get(&self) -> Option { self.0.lock().rtt } } @@ -542,23 +540,7 @@ impl Active { log::trace!("{}: creating new outbound stream", self.id); let id = self.next_stream_id()?; - // TODO: I don't think we want this with a dynamic receive window! - // - // let extra_credit = self.config.receive_window - DEFAULT_CREDIT; - // - // if extra_credit > 0 { - // let mut frame = Frame::window_update(id, extra_credit); - // frame.header_mut().syn(); - // log::trace!("{}/{}: sending initial {}", self.id, id, frame.header()); - // self.pending_frames.push_back(frame.into()); - // } - let mut stream = self.make_new_outbound_stream(id); - - // TODO: I don't think we want this with a dynamic receive window! - // if extra_credit == 0 { - // stream.set_flag(stream::Flag::Syn) - // } stream.set_flag(stream::Flag::Syn); log::debug!("{}: new outbound {} of {}", self.id, stream, self); diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 6eb7b3a9..9302a2cc 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -291,7 +291,7 @@ impl Stream { // https://docs.google.com/document/d/1F2YfdDXKpy20WVKJueEf4abn_LVZHhMUMS5gX6Pgjl4/edit?usp=sharing if self .rtt - .rtt() + .get() .map(|rtt| self.last_window_update.elapsed() < rtt * 2) .unwrap_or(false) { From 6630c9da5b9f30256634a741d72b1a070eeead0b Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 24 Nov 2023 20:49:20 +0100 Subject: [PATCH 12/39] Revert to u32 and add quickcheck --- Cargo.toml | 2 +- quickcheck-ext/Cargo.toml | 13 ++ quickcheck-ext/src/lib.rs | 46 +++++++ test-harness/src/lib.rs | 1 + yamux/Cargo.toml | 2 +- yamux/src/connection.rs | 78 +++++++++-- yamux/src/connection/stream.rs | 236 +++++++++++++++++++++++++++++---- yamux/src/frame.rs | 6 +- yamux/src/lib.rs | 37 +++++- 9 files changed, 372 insertions(+), 49 deletions(-) create mode 100644 quickcheck-ext/Cargo.toml create mode 100644 quickcheck-ext/src/lib.rs diff --git a/Cargo.toml b/Cargo.toml index d28c2d96..7c60ecb3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,3 +1,3 @@ [workspace] -members = ["yamux", "test-harness"] +members = ["yamux", "test-harness", "quickcheck-ext"] resolver = "2" diff --git a/quickcheck-ext/Cargo.toml b/quickcheck-ext/Cargo.toml new file mode 100644 index 00000000..50aed154 --- /dev/null +++ b/quickcheck-ext/Cargo.toml @@ -0,0 +1,13 @@ +[package] +name = "quickcheck-ext" +version = "0.1.0" +edition = "2021" +publish = false +license = "Unlicense/MIT" + +[package.metadata.release] +release = false + +[dependencies] +quickcheck = "1" +num-traits = "0.2" diff --git a/quickcheck-ext/src/lib.rs b/quickcheck-ext/src/lib.rs new file mode 100644 index 00000000..a3b9ce26 --- /dev/null +++ b/quickcheck-ext/src/lib.rs @@ -0,0 +1,46 @@ +#![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] + +pub use quickcheck::*; + +use core::ops::Range; +use num_traits::sign::Unsigned; + +pub trait GenRange { + fn gen_range(&mut self, _range: Range) -> T; + + fn gen_index(&mut self, ubound: usize) -> usize { + if ubound <= (core::u32::MAX as usize) { + self.gen_range(0..ubound as u32) as usize + } else { + self.gen_range(0..ubound) + } + } +} + +impl GenRange for Gen { + fn gen_range(&mut self, range: Range) -> T { + ::arbitrary(self) % (range.end - range.start) + range.start + } +} + +pub trait SliceRandom { + fn shuffle(&mut self, arr: &mut [T]); + fn choose_multiple<'a, T>( + &mut self, + arr: &'a [T], + amount: usize, + ) -> std::iter::Take> { + let mut v: Vec<&T> = arr.iter().collect(); + self.shuffle(&mut v); + v.into_iter().take(amount) + } +} + +impl SliceRandom for Gen { + fn shuffle(&mut self, arr: &mut [T]) { + for i in (1..arr.len()).rev() { + // invariant: elements with index > i have been locked in place. + arr.swap(i, self.gen_index(i + 1)); + } + } +} diff --git a/test-harness/src/lib.rs b/test-harness/src/lib.rs index 14509d6e..b78814af 100644 --- a/test-harness/src/lib.rs +++ b/test-harness/src/lib.rs @@ -445,6 +445,7 @@ impl Arbitrary for Msg { #[derive(Clone, Debug)] pub struct TestConfig(pub Config); +// TODO: use new direct implementation? impl Arbitrary for TestConfig { fn arbitrary(g: &mut Gen) -> Self { let mut c = Config::default(); diff --git a/yamux/Cargo.toml b/yamux/Cargo.toml index e919d679..3a6360e5 100644 --- a/yamux/Cargo.toml +++ b/yamux/Cargo.toml @@ -20,4 +20,4 @@ pin-project = "1.1.0" [dev-dependencies] futures = { version = "0.3.12", default-features = false, features = ["executor"] } -quickcheck = "1.0" +quickcheck = { package = "quickcheck-ext", path = "../quickcheck-ext" } \ No newline at end of file diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 3994c02f..ac5a3a1f 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -293,6 +293,7 @@ struct Active { rtt: Rtt, + // TODO: Document accumulated_max_stream_windows: Arc>, } @@ -367,12 +368,43 @@ impl Rtt { } } +#[cfg(test)] +impl quickcheck::Arbitrary for Rtt { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + Self(Arc::new(Mutex::new(RttInner::arbitrary(g)))) + } +} + #[derive(Debug)] struct RttInner { state: RttState, rtt: Option, } +#[cfg(test)] +impl Clone for RttInner { + fn clone(&self) -> Self { + Self { + state: self.state.clone(), + rtt: self.rtt.clone(), + } + } +} + +#[cfg(test)] +impl quickcheck::Arbitrary for RttInner { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + Self { + state: RttState::arbitrary(g), + rtt: if bool::arbitrary(g) { + Some(Duration::arbitrary(g)) + } else { + None + }, + } + } +} + #[derive(Debug)] enum RttState { Initial, @@ -380,6 +412,38 @@ enum RttState { Waiting { next: Instant }, } +#[cfg(test)] +impl Clone for RttState { + fn clone(&self) -> Self { + match self { + Self::Initial => Self::Initial, + Self::AwaitingPong { sent_at, nonce } => Self::AwaitingPong { + sent_at: sent_at.clone(), + nonce: nonce.clone(), + }, + Self::Waiting { next } => Self::Waiting { next: next.clone() }, + } + } +} + +#[cfg(test)] +impl quickcheck::Arbitrary for RttState { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + use quickcheck::GenRange; + match g.gen_range(0u8..3) { + 0 => RttState::Initial, + 1 => RttState::AwaitingPong { + sent_at: Instant::now(), + nonce: u32::arbitrary(g), + }, + 2 => RttState::Waiting { + next: Instant::now(), + }, + _ => unreachable!(), + } + } +} + /// `Stream` to `Connection` commands. #[derive(Debug)] pub(crate) enum StreamCommand { @@ -730,7 +794,7 @@ impl Active { if let Some(s) = self.streams.get_mut(&stream_id) { let mut shared = s.lock(); - if frame.body().len() > shared.current_receive_window_size as usize { + if frame.body_len() > shared.current_receive_window_size { log::error!( "{}/{}: frame body larger than window of stream", self.id, @@ -741,9 +805,7 @@ impl Active { if is_finish { shared.update_state(self.id, stream_id, State::RecvClosed); } - // TODO - let max_buffer_size = shared.max_receive_window_size as usize; - if shared.buffer.len() >= max_buffer_size { + if shared.buffer.len() >= shared.max_receive_window_size as usize { log::error!( "{}/{}: buffer of stream grows beyond limit", self.id, @@ -814,8 +876,7 @@ impl Active { return Action::Terminate(Frame::protocol_error()); } - // TODO: Is the cast safe? - let credit = frame.header().credit() as usize + DEFAULT_CREDIT; + let credit = frame.header().credit() + DEFAULT_CREDIT; let mut stream = self.make_new_inbound_stream(stream_id, credit); stream.set_flag(stream::Flag::Ack); @@ -830,8 +891,7 @@ impl Active { if let Some(s) = self.streams.get_mut(&stream_id) { let mut shared = s.lock(); - // TODO: Is the cast safe? - shared.current_send_window_size += frame.header().credit() as usize; + shared.current_send_window_size += frame.header().credit(); if is_finish { shared.update_state(self.id, stream_id, State::RecvClosed); } @@ -883,7 +943,7 @@ impl Active { Action::None } - fn make_new_inbound_stream(&mut self, id: StreamId, credit: usize) -> Stream { + fn make_new_inbound_stream(&mut self, id: StreamId, credit: u32) -> Stream { let config = self.config.clone(); let (sender, receiver) = mpsc::channel(10); // 10 is an arbitrary number. diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 9302a2cc..2f0d324c 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -58,6 +58,23 @@ pub enum State { Closed, } +#[cfg(test)] +impl quickcheck::Arbitrary for State { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + use quickcheck::GenRange; + + match g.gen_range(0u8..4) { + 0 => State::Open { + acknowledged: bool::arbitrary(g), + }, + 1 => State::SendClosed, + 2 => State::RecvClosed, + 3 => State::Closed, + _ => unreachable!(), + } + } +} + impl State { /// Can we receive messages over this stream? pub fn can_read(self) -> bool { @@ -81,6 +98,20 @@ pub(crate) enum Flag { Ack, } +#[cfg(test)] +impl quickcheck::Arbitrary for Flag { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + use quickcheck::GenRange; + + match g.gen_range(0u8..2) { + 0 => Flag::None, + 1 => Flag::Syn, + 2 => Flag::Ack, + _ => unreachable!(), + } + } +} + /// A multiplexed Yamux stream. /// /// Streams are created either outbound via [`crate::Connection::poll_new_outbound`] @@ -91,6 +122,7 @@ pub(crate) enum Flag { pub struct Stream { id: StreamId, conn: connection::Id, + // TODO: Why does both stream and shared have the config? config: Arc, sender: mpsc::Sender, flag: Flag, @@ -120,7 +152,7 @@ impl Stream { id: StreamId, conn: connection::Id, config: Arc, - credit: usize, + curent_send_window_size: u32, sender: mpsc::Sender, rtt: Rtt, accumulated_max_stream_windows: Arc>, @@ -131,7 +163,11 @@ impl Stream { config: config.clone(), sender, flag: Flag::None, - shared: Arc::new(Mutex::new(Shared::new(DEFAULT_CREDIT, credit, config))), + shared: Arc::new(Mutex::new(Shared::new( + DEFAULT_CREDIT, + curent_send_window_size, + config, + ))), accumulated_max_stream_windows, rtt, last_window_update: Instant::now(), @@ -230,9 +266,7 @@ impl Stream { return Poll::Ready(Ok(())); }; - // TODO: Handle the case where credit is larger than u32::MAX. Use the smaller and don't - // bump shared.window with the higher. - let mut frame = Frame::window_update(self.id, credit as u32).right(); + let mut frame = Frame::window_update(self.id, credit).right(); self.add_flag(frame.header_mut()); let cmd = StreamCommand::SendFrame(frame); self.sender @@ -246,9 +280,11 @@ impl Stream { /// sending side (remote) via a window update message. /// /// Returns `None` if too small to justify a window update message. - fn next_window_update(&mut self) -> Option { + fn next_window_update(&mut self) -> Option { let mut shared = self.shared.lock(); + println!("1"); + debug_assert!( shared.max_receive_window_size >= shared.current_receive_window_size, "max_receive_window_size: {} current_receive_window_size: {}", @@ -257,13 +293,16 @@ impl Stream { ); let bytes_received = shared.max_receive_window_size - shared.current_receive_window_size; - let mut next_window_update = bytes_received.saturating_sub(shared.buffer.len()); + let mut next_window_update = + bytes_received.saturating_sub(shared.buffer.len().try_into().unwrap_or(u32::MAX)); // Don't send an update in case half or more of the window is still available to the sender. if next_window_update < shared.max_receive_window_size / 2 { return None; } + println!("2"); + log::debug!( "received {} mb in {} seconds ({} mbit/s)", next_window_update as f64 / 1024.0 / 1024.0, @@ -297,34 +336,36 @@ impl Stream { { let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); + debug_assert!( + *accumulated_max_stream_windows + >= (shared.max_receive_window_size - DEFAULT_CREDIT) as usize + ); + // Ideally one can just double it: let mut new_max = shared.max_receive_window_size.saturating_mul(2); // Then one has to consider the configured stream limit: new_max = cmp::min( new_max, - shared - .config - .max_stream_receive_window - .unwrap_or(usize::MAX), + shared.config.max_stream_receive_window.unwrap_or(u32::MAX), ); // Then one has to consider the configured connection limit: new_max = { - let connection_limit = shared.max_receive_window_size + + let connection_limit: usize = dbg!(shared.max_receive_window_size) as usize + // the overall configured conneciton limit - shared.config.max_connection_receive_window + dbg!(shared.config.max_connection_receive_window) // minus the minimum amount of window guaranteed to each stream - - shared.config.max_num_streams * DEFAULT_CREDIT + - dbg!(shared.config.max_num_streams * DEFAULT_CREDIT as usize) // minus the amount of bytes beyond the minimum amount (`DEFAULT_CREDIT`) // already allocated by this and other streams on the connection. - - *accumulated_max_stream_windows; + - dbg!(*accumulated_max_stream_windows); - cmp::min(new_max, connection_limit) + cmp::min(new_max, connection_limit.try_into().unwrap_or(u32::MAX)) }; // Account for the additional credit on the accumulated connection counter. - *accumulated_max_stream_windows += new_max - shared.max_receive_window_size; + *accumulated_max_stream_windows += (new_max - shared.max_receive_window_size) as usize; drop(accumulated_max_stream_windows); log::debug!( @@ -338,12 +379,17 @@ impl Stream { // Recalculate `next_window_update` with the new `max_receive_window_size`. let bytes_received = shared.max_receive_window_size - shared.current_receive_window_size; - next_window_update = bytes_received.saturating_sub(shared.buffer.len()); + next_window_update = + bytes_received.saturating_sub(shared.buffer.len().try_into().unwrap_or(u32::MAX)); } + println!("3"); + self.last_window_update = Instant::now(); shared.current_receive_window_size += next_window_update; + println!("4"); + return Some(next_window_update); } } @@ -483,10 +529,16 @@ impl AsyncWrite for Stream { shared.writer = Some(cx.waker().clone()); return Poll::Pending; } - let k = std::cmp::min(shared.current_send_window_size as usize, buf.len()); - let k = std::cmp::min(k, self.config.split_send_size); + let k = std::cmp::min( + shared.current_send_window_size, + buf.len().try_into().unwrap_or(u32::MAX), + ); + let k = std::cmp::min( + k, + self.config.split_send_size.try_into().unwrap_or(u32::MAX), + ); shared.current_send_window_size = shared.current_send_window_size.saturating_sub(k); - Vec::from(&buf[..k]) + Vec::from(&buf[..k as usize]) }; let n = body.len(); let mut frame = Frame::data(self.id, body).expect("body <= u32::MAX").left(); @@ -542,32 +594,91 @@ impl AsyncWrite for Stream { impl Drop for Stream { fn drop(&mut self) { - *self.accumulated_max_stream_windows.lock() -= - self.shared.lock().max_receive_window_size - DEFAULT_CREDIT; + println!("drop"); + let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); + let max_receive_window_size = self.shared.lock().max_receive_window_size; + + debug_assert!( + *accumulated_max_stream_windows >= (max_receive_window_size - DEFAULT_CREDIT) as usize, + "{accumulated_max_stream_windows} {max_receive_window_size}" + ); + + *accumulated_max_stream_windows -= (max_receive_window_size - DEFAULT_CREDIT) as usize; } } #[derive(Debug)] pub(crate) struct Shared { state: State, - pub(crate) current_receive_window_size: usize, - pub(crate) max_receive_window_size: usize, - pub(crate) current_send_window_size: usize, + pub(crate) current_receive_window_size: u32, + pub(crate) max_receive_window_size: u32, + pub(crate) current_send_window_size: u32, pub(crate) buffer: Chunks, pub(crate) reader: Option, pub(crate) writer: Option, config: Arc, } +#[cfg(test)] +impl Clone for Shared { + fn clone(&self) -> Self { + Self { + state: self.state.clone(), + current_receive_window_size: self.current_receive_window_size.clone(), + max_receive_window_size: self.max_receive_window_size.clone(), + current_send_window_size: self.current_send_window_size.clone(), + buffer: Chunks::new(), + reader: self.reader.clone(), + writer: self.writer.clone(), + config: self.config.clone(), + } + } +} + +#[cfg(test)] +impl quickcheck::Arbitrary for Shared { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + use quickcheck::GenRange; + + let config = Arc::new(Config::arbitrary(g)); + + let lower = DEFAULT_CREDIT; + let higher = cmp::min( + config.max_stream_receive_window.unwrap_or(u32::MAX), + (DEFAULT_CREDIT as usize + config.max_connection_receive_window + - (config.max_num_streams * (DEFAULT_CREDIT as usize))) + .try_into() + .unwrap_or(u32::MAX), + ) + .saturating_add(1); + let max_receive_window_size = g.gen_range(dbg!(lower)..dbg!(higher)); + + Self { + state: State::arbitrary(g), + current_receive_window_size: g.gen_range(0..max_receive_window_size), + max_receive_window_size, + current_send_window_size: g.gen_range(0..u32::MAX), + buffer: Chunks::new(), + reader: None, + writer: None, + config, + } + } +} + impl Shared { - fn new(window: usize, credit: usize, config: Arc) -> Self { + fn new( + current_receive_window_size: u32, + current_send_window_size: u32, + config: Arc, + ) -> Self { Shared { state: State::Open { acknowledged: false, }, - current_receive_window_size: window, + current_receive_window_size, max_receive_window_size: DEFAULT_CREDIT, - current_send_window_size: credit, + current_send_window_size, buffer: Chunks::new(), reader: None, writer: None, @@ -625,3 +736,70 @@ impl Shared { ) } } + +#[cfg(test)] +mod tests { + use std::time::Duration; + + use super::*; + use quickcheck::{Arbitrary, Gen, QuickCheck}; + + impl Clone for Stream { + fn clone(&self) -> Self { + Self { + id: self.id.clone(), + conn: self.conn.clone(), + config: self.config.clone(), + sender: self.sender.clone(), + flag: self.flag.clone(), + shared: Arc::new(Mutex::new(self.shared.lock().clone())), + accumulated_max_stream_windows: Arc::new(Mutex::new( + self.accumulated_max_stream_windows.lock().clone(), + )), + rtt: self.rtt.clone(), + last_window_update: self.last_window_update.clone(), + } + } + } + + impl Arbitrary for Stream { + fn arbitrary(g: &mut Gen) -> Self { + use quickcheck::GenRange; + + let shared = Shared::arbitrary(g); + + Self { + id: StreamId::new(0), + conn: connection::Id::random(), + sender: futures::channel::mpsc::channel(0).0, + flag: Flag::arbitrary(g), + accumulated_max_stream_windows: Arc::new(Mutex::new(g.gen_range( + dbg!((shared.max_receive_window_size - DEFAULT_CREDIT) as usize) + ..(shared.config.max_connection_receive_window + - shared.config.max_num_streams * DEFAULT_CREDIT as usize + + 1), + ))), + rtt: Rtt::arbitrary(g), + last_window_update: Instant::now() + - Duration::from_secs(g.gen_range(0..(60 * 60 * 24))), + config: shared.config.clone(), + shared: Arc::new(Mutex::new(shared)), + } + } + } + + #[test] + fn next_window_update() { + fn property(mut stream: Stream) { + println!("next: {stream}"); + println!( + "{}, {}", + stream.accumulated_max_stream_windows.lock(), + stream.shared.lock().max_receive_window_size + ); + stream.next_window_update(); + } + + QuickCheck::new().quickcheck(property as fn(_)) + } +} diff --git a/yamux/src/frame.rs b/yamux/src/frame.rs index 09291391..be010a3b 100644 --- a/yamux/src/frame.rs +++ b/yamux/src/frame.rs @@ -112,8 +112,10 @@ impl Frame { &self.body } - pub fn body_len(&self) -> usize { - self.body().len() + pub fn body_len(&self) -> u32 { + // Safe cast since we construct `Frame::`s only with + // `Vec` of length [0, u32::MAX] in `Frame::data` above. + self.body().len() as u32 } pub fn into_body(self) -> Vec { diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index c2e97fb0..94b59abe 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -38,7 +38,7 @@ pub use crate::frame::{ FrameDecodeError, }; -pub const DEFAULT_CREDIT: usize = 256 * 1024; // as per yamux specification +pub const DEFAULT_CREDIT: u32 = 256 * 1024; // as per yamux specification pub type Result = std::result::Result; @@ -63,7 +63,6 @@ const MAX_ACK_BACKLOG: usize = 256; /// https://github.com/paritytech/yamux/issues/100. const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * 1024; - // TODO: Update /// Yamux configuration. /// @@ -77,7 +76,7 @@ const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * 1024; /// - split send size = 16 KiB #[derive(Debug, Clone)] pub struct Config { - max_stream_receive_window: Option, + max_stream_receive_window: Option, max_connection_receive_window: usize, max_num_streams: usize, read_after_close: bool, @@ -90,7 +89,6 @@ impl Default for Config { // TODO: Add rational: given that we have a connection window, ... max_stream_receive_window: None, // TODO: reevaluate default. - // TODO: Add setter. max_connection_receive_window: 1 * 1024 * 1024 * 1024, // TODO max_num_streams: 512, @@ -106,12 +104,13 @@ impl Config { /// # Panics /// /// If the given receive window is < 256 KiB. - pub fn set_max_stream_receive_window(&mut self, n: Option) -> &mut Self { + pub fn set_max_stream_receive_window(&mut self, n: Option) -> &mut Self { self.max_stream_receive_window = n; self.check(); self } + // TODO: Is a usize really needed here? pub fn set_max_connection_receive_window(&mut self, n: usize) -> &mut Self { self.max_connection_receive_window = n; self.check(); @@ -143,8 +142,10 @@ impl Config { // TODO: Consider doing the check on creation, not on each builder method call. fn check(&self) { - assert!(self.max_stream_receive_window.unwrap_or(usize::MAX) >= DEFAULT_CREDIT); - assert!(self.max_connection_receive_window >= self.max_num_streams * DEFAULT_CREDIT); + assert!(self.max_stream_receive_window.unwrap_or(u32::MAX) >= DEFAULT_CREDIT); + assert!( + self.max_connection_receive_window >= self.max_num_streams * DEFAULT_CREDIT as usize + ); } } @@ -157,3 +158,25 @@ static_assertions::const_assert! { static_assertions::const_assert! { std::mem::size_of::() <= std::mem::size_of::() } + +#[cfg(test)] +impl quickcheck::Arbitrary for Config { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + use quickcheck::GenRange; + + let max_num_streams = g.gen_range(0..u16::MAX as usize); + + Config { + max_stream_receive_window: if bool::arbitrary(g) { + Some(g.gen_range(DEFAULT_CREDIT..u32::MAX)) + } else { + None + }, + max_connection_receive_window: g + .gen_range((DEFAULT_CREDIT as usize * max_num_streams)..usize::MAX), + max_num_streams, + read_after_close: bool::arbitrary(g), + split_send_size: g.gen_range(DEFAULT_SPLIT_SEND_SIZE..usize::MAX), + } + } +} From df2e62c7b898bdebc54bae9381351b76e5bb4471 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 25 Nov 2023 10:55:30 +0100 Subject: [PATCH 13/39] Minor clean-ups --- test-harness/src/lib.rs | 3 +-- test-harness/tests/poll_api.rs | 2 +- yamux/src/connection.rs | 11 +++++------ yamux/src/connection/stream.rs | 15 --------------- yamux/src/lib.rs | 1 + 5 files changed, 8 insertions(+), 24 deletions(-) diff --git a/test-harness/src/lib.rs b/test-harness/src/lib.rs index b78814af..afcef3be 100644 --- a/test-harness/src/lib.rs +++ b/test-harness/src/lib.rs @@ -445,13 +445,12 @@ impl Arbitrary for Msg { #[derive(Clone, Debug)] pub struct TestConfig(pub Config); -// TODO: use new direct implementation? impl Arbitrary for TestConfig { fn arbitrary(g: &mut Gen) -> Self { let mut c = Config::default(); c.set_read_after_close(Arbitrary::arbitrary(g)); if bool::arbitrary(g) { - c.set_max_stream_receive_window(Some(256 * 1024 + usize::arbitrary(g) % (768 * 1024))); + c.set_max_stream_receive_window(Some(256 * 1024 + u32::arbitrary(g) % (768 * 1024))); } else { c.set_max_stream_receive_window(None); } diff --git a/test-harness/tests/poll_api.rs b/test-harness/tests/poll_api.rs index bb77ed0d..f399d8cc 100644 --- a/test-harness/tests/poll_api.rs +++ b/test-harness/tests/poll_api.rs @@ -63,7 +63,7 @@ fn concurrent_streams() { cfg.set_split_send_size(PAYLOAD_SIZE); // Use a large frame size to speed up the test. // TODO: Rethink these. - cfg.set_max_connection_receive_window(n_streams * DEFAULT_CREDIT); + cfg.set_max_connection_receive_window(n_streams * DEFAULT_CREDIT as usize); cfg.set_max_num_streams(n_streams); Runtime::new().expect("new runtime").block_on(async move { diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index ac5a3a1f..5fede0f0 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -293,7 +293,10 @@ struct Active { rtt: Rtt, - // TODO: Document + /// A stream's `max_stream_receive_window` can grow beyond [`DEFAULT_CREDIT`], see + /// [`Stream::next_window_update`]. This field is the sum of the bytes by which all streams' + /// `max_stream_receive_window` have each exceeded [`DEFAULT_CREDIT`]. Used to enforce + /// [`Config::max_connection_receive_window`]. accumulated_max_stream_windows: Arc>, } @@ -533,9 +536,6 @@ impl Active { fn poll(&mut self, cx: &mut Context<'_>) -> Poll> { loop { if self.socket.poll_ready_unpin(cx).is_ready() { - // TODO: Is this too expensive for it to be at the start of the loop? Could move it - // to the end, but a connection constantly writing and reading would then never send - // a ping. Also might be worth sending ping up front for better latency calc? if let Some(frame) = self.rtt.next_ping() { self.socket.start_send_unpin(frame.into())?; continue; @@ -690,9 +690,8 @@ impl Active { fn on_frame(&mut self, frame: Frame<()>) -> Result> { log::trace!("{}: received: {}", self.id, frame.header()); - // TODO: Clean this change up. if frame.header().flags().contains(header::ACK) - && (frame.header().tag() == Tag::Data || frame.header().tag() == Tag::WindowUpdate) + && matches!(frame.header().tag(), Tag::Data | Tag::WindowUpdate) { let id = frame.header().stream_id(); if let Some(stream) = self.streams.get(&id) { diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 2f0d324c..7df82a96 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -283,8 +283,6 @@ impl Stream { fn next_window_update(&mut self) -> Option { let mut shared = self.shared.lock(); - println!("1"); - debug_assert!( shared.max_receive_window_size >= shared.current_receive_window_size, "max_receive_window_size: {} current_receive_window_size: {}", @@ -301,8 +299,6 @@ impl Stream { return None; } - println!("2"); - log::debug!( "received {} mb in {} seconds ({} mbit/s)", next_window_update as f64 / 1024.0 / 1024.0, @@ -383,13 +379,9 @@ impl Stream { bytes_received.saturating_sub(shared.buffer.len().try_into().unwrap_or(u32::MAX)); } - println!("3"); - self.last_window_update = Instant::now(); shared.current_receive_window_size += next_window_update; - println!("4"); - return Some(next_window_update); } } @@ -594,7 +586,6 @@ impl AsyncWrite for Stream { impl Drop for Stream { fn drop(&mut self) { - println!("drop"); let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); let max_receive_window_size = self.shared.lock().max_receive_window_size; @@ -791,12 +782,6 @@ mod tests { #[test] fn next_window_update() { fn property(mut stream: Stream) { - println!("next: {stream}"); - println!( - "{}, {}", - stream.accumulated_max_stream_windows.lock(), - stream.shared.lock().max_receive_window_size - ); stream.next_window_update(); } diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index 94b59abe..1d6a5070 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -111,6 +111,7 @@ impl Config { } // TODO: Is a usize really needed here? + // TODO: Should this be an option? pub fn set_max_connection_receive_window(&mut self, n: usize) -> &mut Self { self.max_connection_receive_window = n; self.check(); From 3fda972136f19dd8ef8a6f21d6a0be1d0ed4b927 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 25 Nov 2023 11:31:30 +0100 Subject: [PATCH 14/39] Add assertions --- yamux/src/connection/stream.rs | 206 ++++++++++++++++++--------------- 1 file changed, 113 insertions(+), 93 deletions(-) diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 7df82a96..3f6eb2a2 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -122,7 +122,6 @@ impl quickcheck::Arbitrary for Flag { pub struct Stream { id: StreamId, conn: connection::Id, - // TODO: Why does both stream and shared have the config? config: Arc, sender: mpsc::Sender, flag: Flag, @@ -166,7 +165,6 @@ impl Stream { shared: Arc::new(Mutex::new(Shared::new( DEFAULT_CREDIT, curent_send_window_size, - config, ))), accumulated_max_stream_windows, rtt, @@ -188,11 +186,7 @@ impl Stream { config: config.clone(), sender, flag: Flag::None, - shared: Arc::new(Mutex::new(Shared::new( - DEFAULT_CREDIT, - DEFAULT_CREDIT, - config, - ))), + shared: Arc::new(Mutex::new(Shared::new(DEFAULT_CREDIT, DEFAULT_CREDIT))), accumulated_max_stream_windows, rtt, last_window_update: Instant::now(), @@ -281,14 +275,47 @@ impl Stream { /// /// Returns `None` if too small to justify a window update message. fn next_window_update(&mut self) -> Option { - let mut shared = self.shared.lock(); + let debug_assert = || { + if !cfg!(debug_assertions) { + return; + } - debug_assert!( - shared.max_receive_window_size >= shared.current_receive_window_size, - "max_receive_window_size: {} current_receive_window_size: {}", - shared.max_receive_window_size, - shared.current_receive_window_size - ); + let config = &self.config; + let shared = self.shared.lock(); + let accumulated_max_stream_windows = *self.accumulated_max_stream_windows.lock(); + let rtt = self.rtt.get(); + + assert!( + shared.current_receive_window_size <= shared.max_receive_window_size, + "The current window never exceeds the maximum." + ); + assert!( + shared.max_receive_window_size + <= config.max_stream_receive_window.unwrap_or(u32::MAX), + "The maximum never exceeds the configured maximum." + ); + assert!( + (shared.max_receive_window_size - DEFAULT_CREDIT) as usize + <= config.max_connection_receive_window + - config.max_num_streams * DEFAULT_CREDIT as usize, + "The maximum never exceeds its maximum portion of the configured connection limit." + ); + assert!( + (shared.max_receive_window_size - DEFAULT_CREDIT) as usize + <= accumulated_max_stream_windows, + "The amount by which the stream maximum exceeds DEFAULT_CREDIT is tracked in accumulated_max_stream_windows." + ); + if rtt.is_none() { + assert_eq!( + shared.max_receive_window_size, DEFAULT_CREDIT, + "The maximum is only increased iff an rtt measurement is available." + ); + } + }; + + debug_assert(); + + let mut shared = self.shared.lock(); let bytes_received = shared.max_receive_window_size - shared.current_receive_window_size; let mut next_window_update = @@ -299,7 +326,7 @@ impl Stream { return None; } - log::debug!( + log::trace!( "received {} mb in {} seconds ({} mbit/s)", next_window_update as f64 / 1024.0 / 1024.0, self.last_window_update.elapsed().as_secs_f64(), @@ -332,30 +359,25 @@ impl Stream { { let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); - debug_assert!( - *accumulated_max_stream_windows - >= (shared.max_receive_window_size - DEFAULT_CREDIT) as usize - ); - // Ideally one can just double it: let mut new_max = shared.max_receive_window_size.saturating_mul(2); // Then one has to consider the configured stream limit: new_max = cmp::min( new_max, - shared.config.max_stream_receive_window.unwrap_or(u32::MAX), + self.config.max_stream_receive_window.unwrap_or(u32::MAX), ); // Then one has to consider the configured connection limit: new_max = { - let connection_limit: usize = dbg!(shared.max_receive_window_size) as usize + + let connection_limit: usize = shared.max_receive_window_size as usize + // the overall configured conneciton limit - dbg!(shared.config.max_connection_receive_window) + self.config.max_connection_receive_window // minus the minimum amount of window guaranteed to each stream - - dbg!(shared.config.max_num_streams * DEFAULT_CREDIT as usize) + - self.config.max_num_streams * DEFAULT_CREDIT as usize // minus the amount of bytes beyond the minimum amount (`DEFAULT_CREDIT`) // already allocated by this and other streams on the connection. - - dbg!(*accumulated_max_stream_windows); + - *accumulated_max_stream_windows; cmp::min(new_max, connection_limit.try_into().unwrap_or(u32::MAX)) }; @@ -382,6 +404,8 @@ impl Stream { self.last_window_update = Instant::now(); shared.current_receive_window_size += next_window_update; + debug_assert(); + return Some(next_window_update); } } @@ -598,6 +622,67 @@ impl Drop for Stream { } } +#[cfg(test)] +impl Clone for Stream { + fn clone(&self) -> Self { + Self { + id: self.id.clone(), + conn: self.conn.clone(), + config: self.config.clone(), + sender: self.sender.clone(), + flag: self.flag.clone(), + shared: Arc::new(Mutex::new(self.shared.lock().clone())), + accumulated_max_stream_windows: Arc::new(Mutex::new( + self.accumulated_max_stream_windows.lock().clone(), + )), + rtt: self.rtt.clone(), + last_window_update: self.last_window_update.clone(), + } + } +} + +#[cfg(test)] +impl quickcheck::Arbitrary for Stream { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + use quickcheck::GenRange; + + let mut shared = Shared::arbitrary(g); + let config = Arc::new(Config::arbitrary(g)); + + // Update `shared` to align with `config`. + shared.max_receive_window_size = g.gen_range( + DEFAULT_CREDIT + ..cmp::min( + config.max_stream_receive_window.unwrap_or(u32::MAX), + (DEFAULT_CREDIT as usize + config.max_connection_receive_window + - (config.max_num_streams * (DEFAULT_CREDIT as usize))) + .try_into() + .unwrap_or(u32::MAX), + ) + .saturating_add(1), + ); + shared.current_receive_window_size = g.gen_range(0..shared.max_receive_window_size); + + Self { + id: StreamId::new(0), + conn: connection::Id::random(), + sender: futures::channel::mpsc::channel(0).0, + flag: Flag::arbitrary(g), + accumulated_max_stream_windows: Arc::new(Mutex::new(g.gen_range( + (shared.max_receive_window_size - DEFAULT_CREDIT) as usize + ..(config.max_connection_receive_window + - config.max_num_streams * DEFAULT_CREDIT as usize + + 1), + ))), + rtt: Rtt::arbitrary(g), + last_window_update: Instant::now() + - std::time::Duration::from_secs(g.gen_range(0..(60 * 60 * 24))), + config, + shared: Arc::new(Mutex::new(shared)), + } + } +} + #[derive(Debug)] pub(crate) struct Shared { state: State, @@ -607,7 +692,6 @@ pub(crate) struct Shared { pub(crate) buffer: Chunks, pub(crate) reader: Option, pub(crate) writer: Option, - config: Arc, } #[cfg(test)] @@ -621,7 +705,6 @@ impl Clone for Shared { buffer: Chunks::new(), reader: self.reader.clone(), writer: self.writer.clone(), - config: self.config.clone(), } } } @@ -631,18 +714,7 @@ impl quickcheck::Arbitrary for Shared { fn arbitrary(g: &mut quickcheck::Gen) -> Self { use quickcheck::GenRange; - let config = Arc::new(Config::arbitrary(g)); - - let lower = DEFAULT_CREDIT; - let higher = cmp::min( - config.max_stream_receive_window.unwrap_or(u32::MAX), - (DEFAULT_CREDIT as usize + config.max_connection_receive_window - - (config.max_num_streams * (DEFAULT_CREDIT as usize))) - .try_into() - .unwrap_or(u32::MAX), - ) - .saturating_add(1); - let max_receive_window_size = g.gen_range(dbg!(lower)..dbg!(higher)); + let max_receive_window_size = g.gen_range(DEFAULT_CREDIT..u32::MAX); Self { state: State::arbitrary(g), @@ -652,17 +724,12 @@ impl quickcheck::Arbitrary for Shared { buffer: Chunks::new(), reader: None, writer: None, - config, } } } impl Shared { - fn new( - current_receive_window_size: u32, - current_send_window_size: u32, - config: Arc, - ) -> Self { + fn new(current_receive_window_size: u32, current_send_window_size: u32) -> Self { Shared { state: State::Open { acknowledged: false, @@ -673,7 +740,6 @@ impl Shared { buffer: Chunks::new(), reader: None, writer: None, - config, } } @@ -730,54 +796,8 @@ impl Shared { #[cfg(test)] mod tests { - use std::time::Duration; - use super::*; - use quickcheck::{Arbitrary, Gen, QuickCheck}; - - impl Clone for Stream { - fn clone(&self) -> Self { - Self { - id: self.id.clone(), - conn: self.conn.clone(), - config: self.config.clone(), - sender: self.sender.clone(), - flag: self.flag.clone(), - shared: Arc::new(Mutex::new(self.shared.lock().clone())), - accumulated_max_stream_windows: Arc::new(Mutex::new( - self.accumulated_max_stream_windows.lock().clone(), - )), - rtt: self.rtt.clone(), - last_window_update: self.last_window_update.clone(), - } - } - } - - impl Arbitrary for Stream { - fn arbitrary(g: &mut Gen) -> Self { - use quickcheck::GenRange; - - let shared = Shared::arbitrary(g); - - Self { - id: StreamId::new(0), - conn: connection::Id::random(), - sender: futures::channel::mpsc::channel(0).0, - flag: Flag::arbitrary(g), - accumulated_max_stream_windows: Arc::new(Mutex::new(g.gen_range( - dbg!((shared.max_receive_window_size - DEFAULT_CREDIT) as usize) - ..(shared.config.max_connection_receive_window - - shared.config.max_num_streams * DEFAULT_CREDIT as usize - + 1), - ))), - rtt: Rtt::arbitrary(g), - last_window_update: Instant::now() - - Duration::from_secs(g.gen_range(0..(60 * 60 * 24))), - config: shared.config.clone(), - shared: Arc::new(Mutex::new(shared)), - } - } - } + use quickcheck::QuickCheck; #[test] fn next_window_update() { From 2892b8d7bf4fad5bc85eef35223d850b2de26b40 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 25 Nov 2023 11:57:40 +0100 Subject: [PATCH 15/39] Fix deadlock --- yamux/src/connection/stream.rs | 34 ++++++++++++++++++++-------------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 3f6eb2a2..93891688 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -282,8 +282,8 @@ impl Stream { let config = &self.config; let shared = self.shared.lock(); - let accumulated_max_stream_windows = *self.accumulated_max_stream_windows.lock(); let rtt = self.rtt.get(); + let accumulated_max_stream_windows = *self.accumulated_max_stream_windows.lock(); assert!( shared.current_receive_window_size <= shared.max_receive_window_size, @@ -403,6 +403,7 @@ impl Stream { self.last_window_update = Instant::now(); shared.current_receive_window_size += next_window_update; + drop(shared); debug_assert(); @@ -610,8 +611,8 @@ impl AsyncWrite for Stream { impl Drop for Stream { fn drop(&mut self) { - let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); let max_receive_window_size = self.shared.lock().max_receive_window_size; + let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); debug_assert!( *accumulated_max_stream_windows >= (max_receive_window_size - DEFAULT_CREDIT) as usize, @@ -648,19 +649,24 @@ impl quickcheck::Arbitrary for Stream { let mut shared = Shared::arbitrary(g); let config = Arc::new(Config::arbitrary(g)); + let rtt = Rtt::arbitrary(g); - // Update `shared` to align with `config`. - shared.max_receive_window_size = g.gen_range( + // Update `shared` to align with `config` and rtt. + shared.max_receive_window_size = if rtt.get().is_none() { DEFAULT_CREDIT - ..cmp::min( - config.max_stream_receive_window.unwrap_or(u32::MAX), - (DEFAULT_CREDIT as usize + config.max_connection_receive_window - - (config.max_num_streams * (DEFAULT_CREDIT as usize))) - .try_into() - .unwrap_or(u32::MAX), - ) - .saturating_add(1), - ); + } else { + g.gen_range( + DEFAULT_CREDIT + ..cmp::min( + config.max_stream_receive_window.unwrap_or(u32::MAX), + (DEFAULT_CREDIT as usize + config.max_connection_receive_window + - (config.max_num_streams * (DEFAULT_CREDIT as usize))) + .try_into() + .unwrap_or(u32::MAX), + ) + .saturating_add(1), + ) + }; shared.current_receive_window_size = g.gen_range(0..shared.max_receive_window_size); Self { @@ -674,7 +680,7 @@ impl quickcheck::Arbitrary for Stream { - config.max_num_streams * DEFAULT_CREDIT as usize + 1), ))), - rtt: Rtt::arbitrary(g), + rtt, last_window_update: Instant::now() - std::time::Duration::from_secs(g.gen_range(0..(60 * 60 * 24))), config, From 85aef76d1363050749b4e46bcfb284e6b768f536 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 27 Nov 2023 11:08:55 +0100 Subject: [PATCH 16/39] Move rtt into own module --- test-harness/Cargo.toml | 3 +- yamux/Cargo.toml | 2 +- yamux/src/connection.rs | 158 +----------------------------- yamux/src/connection/rtt.rs | 170 +++++++++++++++++++++++++++++++++ yamux/src/connection/stream.rs | 10 +- 5 files changed, 181 insertions(+), 162 deletions(-) create mode 100644 yamux/src/connection/rtt.rs diff --git a/test-harness/Cargo.toml b/test-harness/Cargo.toml index 376d2900..4a3d6daa 100644 --- a/test-harness/Cargo.toml +++ b/test-harness/Cargo.toml @@ -7,7 +7,7 @@ publish = false [dependencies] yamux = { path = "../yamux" } futures = "0.3.4" -quickcheck = "1.0" +quickcheck = { package = "quickcheck-ext", path = "../quickcheck-ext" } tokio = { version = "1.0", features = ["net", "rt-multi-thread", "macros", "time"] } tokio-util = { version = "0.7", features = ["compat"] } anyhow = "1" @@ -17,7 +17,6 @@ log = "0.4.17" criterion = "0.5" env_logger = "0.10" futures = "0.3.4" -quickcheck = "1.0" tokio = { version = "1.0", features = ["net", "rt-multi-thread", "macros", "time"] } tokio-util = { version = "0.7", features = ["compat"] } constrained-connection = "0.1" diff --git a/yamux/Cargo.toml b/yamux/Cargo.toml index 3a6360e5..3cac8f02 100644 --- a/yamux/Cargo.toml +++ b/yamux/Cargo.toml @@ -20,4 +20,4 @@ pin-project = "1.1.0" [dev-dependencies] futures = { version = "0.3.12", default-features = false, features = ["executor"] } -quickcheck = { package = "quickcheck-ext", path = "../quickcheck-ext" } \ No newline at end of file +quickcheck = { package = "quickcheck-ext", path = "../quickcheck-ext" } diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 5fede0f0..63cf0c6c 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -14,6 +14,7 @@ mod cleanup; mod closing; +mod rtt; mod stream; use crate::tagged_stream::TaggedStream; @@ -32,13 +33,10 @@ use nohash_hasher::IntMap; use parking_lot::Mutex; use std::collections::VecDeque; use std::task::{Context, Waker}; -use std::time::{Duration, Instant}; use std::{fmt, sync::Arc, task::Poll}; pub use stream::{Packet, State, Stream}; -const PING_INTERVAL: Duration = Duration::from_secs(10); - /// How the connection is used. #[derive(Copy, Clone, Debug, Hash, PartialEq, Eq)] pub enum Mode { @@ -291,7 +289,7 @@ struct Active { pending_frames: VecDeque>, new_outbound_stream_waker: Option, - rtt: Rtt, + rtt: rtt::Rtt, /// A stream's `max_stream_receive_window` can grow beyond [`DEFAULT_CREDIT`], see /// [`Stream::next_window_update`]. This field is the sum of the bytes by which all streams' @@ -299,154 +297,6 @@ struct Active { /// [`Config::max_connection_receive_window`]. accumulated_max_stream_windows: Arc>, } - -#[derive(Clone, Debug)] -pub struct Rtt(Arc>); - -impl Rtt { - fn new() -> Self { - Self(Arc::new(Mutex::new(RttInner { - rtt: None, - state: RttState::Initial, - }))) - } - - fn next_ping(&mut self) -> Option> { - let state = &mut self.0.lock().state; - - match state { - RttState::Initial => {} - RttState::AwaitingPong { .. } => return None, - RttState::Waiting { next } => { - if *next > Instant::now() { - return None; - } - } - } - - let nonce = rand::random(); - *state = RttState::AwaitingPong { - sent_at: Instant::now(), - nonce, - }; - log::debug!("sending ping {nonce}"); - Some(Frame::ping(nonce)) - } - - fn handle_pong(&mut self, received_nonce: u32) -> Action { - let inner = &mut self.0.lock(); - - let (sent_at, expected_nonce) = match inner.state { - RttState::Initial | RttState::Waiting { .. } => { - log::error!("received unexpected pong {}", received_nonce); - return Action::Terminate(Frame::protocol_error()); - } - RttState::AwaitingPong { sent_at, nonce } => (sent_at, nonce), - }; - - if received_nonce != expected_nonce { - log::error!( - "received pong with {} but expected {}", - received_nonce, - expected_nonce - ); - return Action::Terminate(Frame::protocol_error()); - } - - inner.rtt = Some(sent_at.elapsed()); - log::debug!( - "received pong {received_nonce}, estimated round-trip-time {}s", - inner.rtt.as_ref().unwrap().as_secs_f64() - ); - - inner.state = RttState::Waiting { - next: Instant::now() + PING_INTERVAL, - }; - - return Action::None; - } - - fn get(&self) -> Option { - self.0.lock().rtt - } -} - -#[cfg(test)] -impl quickcheck::Arbitrary for Rtt { - fn arbitrary(g: &mut quickcheck::Gen) -> Self { - Self(Arc::new(Mutex::new(RttInner::arbitrary(g)))) - } -} - -#[derive(Debug)] -struct RttInner { - state: RttState, - rtt: Option, -} - -#[cfg(test)] -impl Clone for RttInner { - fn clone(&self) -> Self { - Self { - state: self.state.clone(), - rtt: self.rtt.clone(), - } - } -} - -#[cfg(test)] -impl quickcheck::Arbitrary for RttInner { - fn arbitrary(g: &mut quickcheck::Gen) -> Self { - Self { - state: RttState::arbitrary(g), - rtt: if bool::arbitrary(g) { - Some(Duration::arbitrary(g)) - } else { - None - }, - } - } -} - -#[derive(Debug)] -enum RttState { - Initial, - AwaitingPong { sent_at: Instant, nonce: u32 }, - Waiting { next: Instant }, -} - -#[cfg(test)] -impl Clone for RttState { - fn clone(&self) -> Self { - match self { - Self::Initial => Self::Initial, - Self::AwaitingPong { sent_at, nonce } => Self::AwaitingPong { - sent_at: sent_at.clone(), - nonce: nonce.clone(), - }, - Self::Waiting { next } => Self::Waiting { next: next.clone() }, - } - } -} - -#[cfg(test)] -impl quickcheck::Arbitrary for RttState { - fn arbitrary(g: &mut quickcheck::Gen) -> Self { - use quickcheck::GenRange; - match g.gen_range(0u8..3) { - 0 => RttState::Initial, - 1 => RttState::AwaitingPong { - sent_at: Instant::now(), - nonce: u32::arbitrary(g), - }, - 2 => RttState::Waiting { - next: Instant::now(), - }, - _ => unreachable!(), - } - } -} - /// `Stream` to `Connection` commands. #[derive(Debug)] pub(crate) enum StreamCommand { @@ -458,7 +308,7 @@ pub(crate) enum StreamCommand { /// Possible actions as a result of incoming frame handling. #[derive(Debug)] -enum Action { +pub(crate) enum Action { /// Nothing to be done. None, /// A new stream has been opened by the remote. @@ -514,7 +364,7 @@ impl Active { }, pending_frames: VecDeque::default(), new_outbound_stream_waker: None, - rtt: Rtt::new(), + rtt: rtt::Rtt::new(), accumulated_max_stream_windows: Default::default(), } } diff --git a/yamux/src/connection/rtt.rs b/yamux/src/connection/rtt.rs new file mode 100644 index 00000000..395e465c --- /dev/null +++ b/yamux/src/connection/rtt.rs @@ -0,0 +1,170 @@ +// Copyright (c) 2023 Protocol Labs. +// +// Licensed under the Apache License, Version 2.0 or MIT license, at your option. +// +// A copy of the Apache License, Version 2.0 is included in the software as +// LICENSE-APACHE and a copy of the MIT license is included in the software +// as LICENSE-MIT. You may also obtain a copy of the Apache License, Version 2.0 +// at https://www.apache.org/licenses/LICENSE-2.0 and a copy of the MIT license +// at https://opensource.org/licenses/MIT. + +//! Connection round-trip time measurement + +use std::{ + sync::Arc, + time::{Duration, Instant}, +}; + +use parking_lot::Mutex; + +use crate::connection::Action; +use crate::frame::{header::Ping, Frame}; + +const PING_INTERVAL: Duration = Duration::from_secs(10); + +#[derive(Clone, Debug)] +pub(crate) struct Rtt(Arc>); + +impl Rtt { + pub(crate) fn new() -> Self { + Self(Arc::new(Mutex::new(RttInner { + rtt: None, + state: RttState::Initial, + }))) + } + + pub(crate) fn next_ping(&mut self) -> Option> { + let state = &mut self.0.lock().state; + + match state { + RttState::Initial => {} + RttState::AwaitingPong { .. } => return None, + RttState::Waiting { next } => { + if *next > Instant::now() { + return None; + } + } + } + + let nonce = rand::random(); + *state = RttState::AwaitingPong { + sent_at: Instant::now(), + nonce, + }; + log::debug!("sending ping {nonce}"); + Some(Frame::ping(nonce)) + } + + pub(crate) fn handle_pong(&mut self, received_nonce: u32) -> Action { + let inner = &mut self.0.lock(); + + let (sent_at, expected_nonce) = match inner.state { + RttState::Initial | RttState::Waiting { .. } => { + log::error!("received unexpected pong {}", received_nonce); + return Action::Terminate(Frame::protocol_error()); + } + RttState::AwaitingPong { sent_at, nonce } => (sent_at, nonce), + }; + + if received_nonce != expected_nonce { + log::error!( + "received pong with {} but expected {}", + received_nonce, + expected_nonce + ); + return Action::Terminate(Frame::protocol_error()); + } + + inner.rtt = Some(sent_at.elapsed()); + log::debug!( + "received pong {received_nonce}, estimated round-trip-time {}s", + inner.rtt.as_ref().unwrap().as_secs_f64() + ); + + inner.state = RttState::Waiting { + next: Instant::now() + PING_INTERVAL, + }; + + return Action::None; + } + + pub(crate) fn get(&self) -> Option { + self.0.lock().rtt + } +} + +#[cfg(test)] +impl quickcheck::Arbitrary for Rtt { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + Self(Arc::new(Mutex::new(RttInner::arbitrary(g)))) + } +} + +#[derive(Debug)] +struct RttInner { + state: RttState, + rtt: Option, +} + +#[cfg(test)] +impl Clone for RttInner { + fn clone(&self) -> Self { + Self { + state: self.state.clone(), + rtt: self.rtt.clone(), + } + } +} + +#[cfg(test)] +impl quickcheck::Arbitrary for RttInner { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + Self { + state: RttState::arbitrary(g), + rtt: if bool::arbitrary(g) { + Some(Duration::arbitrary(g)) + } else { + None + }, + } + } +} + +#[derive(Debug)] +enum RttState { + Initial, + AwaitingPong { sent_at: Instant, nonce: u32 }, + Waiting { next: Instant }, +} + +#[cfg(test)] +impl Clone for RttState { + fn clone(&self) -> Self { + match self { + Self::Initial => Self::Initial, + Self::AwaitingPong { sent_at, nonce } => Self::AwaitingPong { + sent_at: sent_at.clone(), + nonce: nonce.clone(), + }, + Self::Waiting { next } => Self::Waiting { next: next.clone() }, + } + } +} + +#[cfg(test)] +impl quickcheck::Arbitrary for RttState { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + use quickcheck::GenRange; + match g.gen_range(0u8..3) { + 0 => RttState::Initial, + 1 => RttState::AwaitingPong { + sent_at: Instant::now(), + nonce: u32::arbitrary(g), + }, + 2 => RttState::Waiting { + next: Instant::now(), + }, + _ => unreachable!(), + } + } +} diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 93891688..621644fa 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -11,7 +11,7 @@ use crate::frame::header::ACK; use crate::{ chunks::Chunks, - connection::{self, Rtt, StreamCommand}, + connection::{self, rtt, StreamCommand}, frame::{ header::{Data, Header, StreamId, WindowUpdate}, Frame, @@ -127,7 +127,7 @@ pub struct Stream { flag: Flag, shared: Arc>, accumulated_max_stream_windows: Arc>, - rtt: Rtt, + rtt: rtt::Rtt, last_window_update: Instant, } @@ -153,7 +153,7 @@ impl Stream { config: Arc, curent_send_window_size: u32, sender: mpsc::Sender, - rtt: Rtt, + rtt: rtt::Rtt, accumulated_max_stream_windows: Arc>, ) -> Self { Self { @@ -177,7 +177,7 @@ impl Stream { conn: connection::Id, config: Arc, sender: mpsc::Sender, - rtt: Rtt, + rtt: rtt::Rtt, accumulated_max_stream_windows: Arc>, ) -> Self { Self { @@ -649,7 +649,7 @@ impl quickcheck::Arbitrary for Stream { let mut shared = Shared::arbitrary(g); let config = Arc::new(Config::arbitrary(g)); - let rtt = Rtt::arbitrary(g); + let rtt = rtt::Rtt::arbitrary(g); // Update `shared` to align with `config` and rtt. shared.max_receive_window_size = if rtt.get().is_none() { From a43fbee02dfd3554a042d79841b506bd2beb4ce2 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 27 Nov 2023 13:38:20 +0100 Subject: [PATCH 17/39] Improve docs --- yamux/src/lib.rs | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index 1d6a5070..6d8ba28d 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -99,7 +99,15 @@ impl Default for Config { } impl Config { - /// Set the receive window per stream (must be >= 256 KiB). + /// Set the maximum receive window per stream . + /// + /// Must be `>= 256 KiB`. + /// + /// The window of a stream starts at 256 KiB and is increased (auto-tuned) based on the + /// connection's round-trip time and the stream's bandwidth (striving for the + /// bandwidth-delay-product). + /// + /// Set to `None` to disable the per stream maximum receive window. /// /// # Panics /// @@ -112,13 +120,16 @@ impl Config { // TODO: Is a usize really needed here? // TODO: Should this be an option? + /// Set the maximum sum of all stream receive windows per connection. + /// + /// Must be `>= 256 KiB * max_num_streams` to allow each stream the Yamux default window size. pub fn set_max_connection_receive_window(&mut self, n: usize) -> &mut Self { self.max_connection_receive_window = n; self.check(); self } - /// Set the max. number of streams. + /// Set the max. number of streams per connection. pub fn set_max_num_streams(&mut self, n: usize) -> &mut Self { self.max_num_streams = n; self.check(); From bb0c6a51550019837cc53af7018baf3b990b8582 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 27 Nov 2023 21:16:07 +0100 Subject: [PATCH 18/39] Undo test changes --- test-harness/tests/poll_api.rs | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/test-harness/tests/poll_api.rs b/test-harness/tests/poll_api.rs index f399d8cc..b90c1aaf 100644 --- a/test-harness/tests/poll_api.rs +++ b/test-harness/tests/poll_api.rs @@ -12,7 +12,7 @@ use tokio::net::TcpStream; use tokio::runtime::Runtime; use tokio::task; use tokio_util::compat::TokioAsyncReadCompatExt; -use yamux::{Config, Connection, ConnectionError, Mode, DEFAULT_CREDIT}; +use yamux::{Config, Connection, ConnectionError, Mode}; #[test] fn prop_config_send_recv_multi() { @@ -62,10 +62,6 @@ fn concurrent_streams() { let mut cfg = Config::default(); cfg.set_split_send_size(PAYLOAD_SIZE); // Use a large frame size to speed up the test. - // TODO: Rethink these. - cfg.set_max_connection_receive_window(n_streams * DEFAULT_CREDIT as usize); - cfg.set_max_num_streams(n_streams); - Runtime::new().expect("new runtime").block_on(async move { let (server, client) = connected_peers(cfg.clone(), cfg, tcp_buffer_sizes) .await @@ -181,9 +177,7 @@ fn prop_config_send_recv_single() { let server = echo_server(server); let client = async { - let stream = future::poll_fn(|cx| client.poll_new_outbound(cx)) - .await - .unwrap(); + let stream = future::poll_fn(|cx| client.poll_new_outbound(cx)).await?; let client_task = noop_server(stream::poll_fn(|cx| client.poll_next_inbound(cx))); future::select(pin!(client_task), pin!(send_on_single_stream(stream, msgs))).await; @@ -193,7 +187,7 @@ fn prop_config_send_recv_single() { Ok(()) }; - futures::future::try_join(server, client).await.unwrap(); + futures::future::try_join(server, client).await?; Ok(()) }) From 5d93aa2d4a447049fb94658241a4344ded4b0981 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 29 Nov 2023 14:31:54 +0100 Subject: [PATCH 19/39] Remove Stream::set_flag --- yamux/src/connection.rs | 9 +++------ yamux/src/connection/stream.rs | 9 ++------- 2 files changed, 5 insertions(+), 13 deletions(-) diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 63cf0c6c..6fceeced 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -454,8 +454,7 @@ impl Active { log::trace!("{}: creating new outbound stream", self.id); let id = self.next_stream_id()?; - let mut stream = self.make_new_outbound_stream(id); - stream.set_flag(stream::Flag::Syn); + let stream = self.make_new_outbound_stream(id); log::debug!("{}: new outbound {} of {}", self.id, stream, self); self.streams.insert(id, stream.clone_shared()); @@ -625,7 +624,7 @@ impl Active { log::error!("{}: maximum number of streams reached", self.id); return Action::Terminate(Frame::internal_error()); } - let mut stream = self.make_new_inbound_stream(stream_id, DEFAULT_CREDIT); + let stream = self.make_new_inbound_stream(stream_id, DEFAULT_CREDIT); { let mut shared = stream.shared(); if is_finish { @@ -636,7 +635,6 @@ impl Active { .saturating_sub(frame.body_len()); shared.buffer.push(frame.into_body()); } - stream.set_flag(stream::Flag::Ack); self.streams.insert(stream_id, stream.clone_shared()); return Action::New(stream); } @@ -726,8 +724,7 @@ impl Active { } let credit = frame.header().credit() + DEFAULT_CREDIT; - let mut stream = self.make_new_inbound_stream(stream_id, credit); - stream.set_flag(stream::Flag::Ack); + let stream = self.make_new_inbound_stream(stream_id, credit); if is_finish { stream diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 621644fa..e120b0ae 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -161,7 +161,7 @@ impl Stream { conn, config: config.clone(), sender, - flag: Flag::None, + flag: Flag::Ack, shared: Arc::new(Mutex::new(Shared::new( DEFAULT_CREDIT, curent_send_window_size, @@ -185,7 +185,7 @@ impl Stream { conn, config: config.clone(), sender, - flag: Flag::None, + flag: Flag::Syn, shared: Arc::new(Mutex::new(Shared::new(DEFAULT_CREDIT, DEFAULT_CREDIT))), accumulated_max_stream_windows, rtt, @@ -211,11 +211,6 @@ impl Stream { self.shared().is_pending_ack() } - /// Set the flag that should be set on the next outbound frame header. - pub(crate) fn set_flag(&mut self, flag: Flag) { - self.flag = flag - } - pub(crate) fn shared(&self) -> MutexGuard<'_, Shared> { self.shared.lock() } From cf76ccf62e5b9ea88cf76885da60b0b335c737e1 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Wed, 29 Nov 2023 15:03:39 +0100 Subject: [PATCH 20/39] Document MAX_FRAME_BODY_LEN --- yamux/src/frame/io.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/yamux/src/frame/io.rs b/yamux/src/frame/io.rs index b2a27b0e..4290e224 100644 --- a/yamux/src/frame/io.rs +++ b/yamux/src/frame/io.rs @@ -20,6 +20,11 @@ use std::{ task::{Context, Poll}, }; +/// Maximum Yamux frame body length +/// +/// Limits the amount of bytes a remote can cause the local node to allocate at once when reading. +/// +/// Chosen based on intuition in past iterations. const MAX_FRAME_BODY_LEN: usize = 1024 * 1024; /// A [`Stream`] and writer of [`Frame`] values. From b9b74db02335ee62660688db406eeaff2ea171d1 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 1 Dec 2023 17:57:16 +0100 Subject: [PATCH 21/39] Introduce flow_control module --- yamux/src/connection.rs | 15 +- yamux/src/connection/stream.rs | 381 +++----------------- yamux/src/connection/stream/flow_control.rs | 319 ++++++++++++++++ 3 files changed, 385 insertions(+), 330 deletions(-) create mode 100644 yamux/src/connection/stream/flow_control.rs diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 6fceeced..144114a9 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -630,9 +630,7 @@ impl Active { if is_finish { shared.update_state(self.id, stream_id, State::RecvClosed); } - shared.current_receive_window_size = shared - .current_receive_window_size - .saturating_sub(frame.body_len()); + shared.consume_receive_window(frame.body_len()); shared.buffer.push(frame.into_body()); } self.streams.insert(stream_id, stream.clone_shared()); @@ -641,7 +639,7 @@ impl Active { if let Some(s) = self.streams.get_mut(&stream_id) { let mut shared = s.lock(); - if frame.body_len() > shared.current_receive_window_size { + if frame.body_len() > shared.current_receive_window_size() { log::error!( "{}/{}: frame body larger than window of stream", self.id, @@ -652,7 +650,8 @@ impl Active { if is_finish { shared.update_state(self.id, stream_id, State::RecvClosed); } - if shared.buffer.len() >= shared.max_receive_window_size as usize { + // TODO: Still relevant? This should include frame.body_len() no? + if shared.buffer.len() >= shared.max_receive_window_size() as usize { log::error!( "{}/{}: buffer of stream grows beyond limit", self.id, @@ -662,9 +661,7 @@ impl Active { header.rst(); return Action::Reset(Frame::new(header)); } - shared.current_receive_window_size = shared - .current_receive_window_size - .saturating_sub(frame.body_len()); + shared.consume_receive_window(frame.body_len()); shared.buffer.push(frame.into_body()); if let Some(w) = shared.reader.take() { w.wake() @@ -737,7 +734,7 @@ impl Active { if let Some(s) = self.streams.get_mut(&stream_id) { let mut shared = s.lock(); - shared.current_send_window_size += frame.header().credit(); + shared.increase_send_window_by(frame.header().credit()); if is_finish { shared.update_state(self.id, stream_id, State::RecvClosed); } diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index e120b0ae..ceba6f5f 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -8,6 +8,7 @@ // at https://www.apache.org/licenses/LICENSE-2.0 and a copy of the MIT license // at https://opensource.org/licenses/MIT. +use crate::connection::rtt::Rtt; use crate::frame::header::ACK; use crate::{ chunks::Chunks, @@ -18,6 +19,7 @@ use crate::{ }, Config, DEFAULT_CREDIT, }; +use flow_control::FlowController; use futures::{ channel::mpsc, future::Either, @@ -25,8 +27,6 @@ use futures::{ ready, SinkExt, }; use parking_lot::{Mutex, MutexGuard}; -use std::cmp; -use std::time::Instant; use std::{ fmt, io, pin::Pin, @@ -34,6 +34,8 @@ use std::{ task::{Context, Poll, Waker}, }; +mod flow_control; + /// The state of a Yamux stream. #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum State { @@ -58,23 +60,6 @@ pub enum State { Closed, } -#[cfg(test)] -impl quickcheck::Arbitrary for State { - fn arbitrary(g: &mut quickcheck::Gen) -> Self { - use quickcheck::GenRange; - - match g.gen_range(0u8..4) { - 0 => State::Open { - acknowledged: bool::arbitrary(g), - }, - 1 => State::SendClosed, - 2 => State::RecvClosed, - 3 => State::Closed, - _ => unreachable!(), - } - } -} - impl State { /// Can we receive messages over this stream? pub fn can_read(self) -> bool { @@ -98,20 +83,6 @@ pub(crate) enum Flag { Ack, } -#[cfg(test)] -impl quickcheck::Arbitrary for Flag { - fn arbitrary(g: &mut quickcheck::Gen) -> Self { - use quickcheck::GenRange; - - match g.gen_range(0u8..2) { - 0 => Flag::None, - 1 => Flag::Syn, - 2 => Flag::Ack, - _ => unreachable!(), - } - } -} - /// A multiplexed Yamux stream. /// /// Streams are created either outbound via [`crate::Connection::poll_new_outbound`] @@ -126,9 +97,6 @@ pub struct Stream { sender: mpsc::Sender, flag: Flag, shared: Arc>, - accumulated_max_stream_windows: Arc>, - rtt: rtt::Rtt, - last_window_update: Instant, } impl fmt::Debug for Stream { @@ -151,7 +119,7 @@ impl Stream { id: StreamId, conn: connection::Id, config: Arc, - curent_send_window_size: u32, + current_send_window_size: u32, sender: mpsc::Sender, rtt: rtt::Rtt, accumulated_max_stream_windows: Arc>, @@ -164,11 +132,11 @@ impl Stream { flag: Flag::Ack, shared: Arc::new(Mutex::new(Shared::new( DEFAULT_CREDIT, - curent_send_window_size, + current_send_window_size, + accumulated_max_stream_windows, + rtt, + config, ))), - accumulated_max_stream_windows, - rtt, - last_window_update: Instant::now(), } } @@ -186,10 +154,13 @@ impl Stream { config: config.clone(), sender, flag: Flag::Syn, - shared: Arc::new(Mutex::new(Shared::new(DEFAULT_CREDIT, DEFAULT_CREDIT))), - accumulated_max_stream_windows, - rtt, - last_window_update: Instant::now(), + shared: Arc::new(Mutex::new(Shared::new( + DEFAULT_CREDIT, + DEFAULT_CREDIT, + accumulated_max_stream_windows, + rtt, + config, + ))), } } @@ -251,7 +222,7 @@ impl Stream { .poll_ready(cx) .map_err(|_| self.write_zero_err())?); - let Some(credit) = self.next_window_update() else { + let Some(credit) = self.shared.lock().next_window_update() else { return Poll::Ready(Ok(())); }; @@ -264,146 +235,6 @@ impl Stream { Poll::Ready(Ok(())) } - - /// Calculate the number of additional window bytes the receiving side (local) should grant the - /// sending side (remote) via a window update message. - /// - /// Returns `None` if too small to justify a window update message. - fn next_window_update(&mut self) -> Option { - let debug_assert = || { - if !cfg!(debug_assertions) { - return; - } - - let config = &self.config; - let shared = self.shared.lock(); - let rtt = self.rtt.get(); - let accumulated_max_stream_windows = *self.accumulated_max_stream_windows.lock(); - - assert!( - shared.current_receive_window_size <= shared.max_receive_window_size, - "The current window never exceeds the maximum." - ); - assert!( - shared.max_receive_window_size - <= config.max_stream_receive_window.unwrap_or(u32::MAX), - "The maximum never exceeds the configured maximum." - ); - assert!( - (shared.max_receive_window_size - DEFAULT_CREDIT) as usize - <= config.max_connection_receive_window - - config.max_num_streams * DEFAULT_CREDIT as usize, - "The maximum never exceeds its maximum portion of the configured connection limit." - ); - assert!( - (shared.max_receive_window_size - DEFAULT_CREDIT) as usize - <= accumulated_max_stream_windows, - "The amount by which the stream maximum exceeds DEFAULT_CREDIT is tracked in accumulated_max_stream_windows." - ); - if rtt.is_none() { - assert_eq!( - shared.max_receive_window_size, DEFAULT_CREDIT, - "The maximum is only increased iff an rtt measurement is available." - ); - } - }; - - debug_assert(); - - let mut shared = self.shared.lock(); - - let bytes_received = shared.max_receive_window_size - shared.current_receive_window_size; - let mut next_window_update = - bytes_received.saturating_sub(shared.buffer.len().try_into().unwrap_or(u32::MAX)); - - // Don't send an update in case half or more of the window is still available to the sender. - if next_window_update < shared.max_receive_window_size / 2 { - return None; - } - - log::trace!( - "received {} mb in {} seconds ({} mbit/s)", - next_window_update as f64 / 1024.0 / 1024.0, - self.last_window_update.elapsed().as_secs_f64(), - next_window_update as f64 / 1024.0 / 1024.0 * 8.0 - / self.last_window_update.elapsed().as_secs_f64() - ); - - // Auto-tuning `max_receive_window_size` - // - // The ideal `max_receive_window_size` is equal to the bandwidth-delay-product (BDP), thus - // allowing the remote sender to exhaust the entire available bandwidth on a single stream. - // Choosing `max_receive_window_size` too small prevents the remote sender from exhausting - // the available bandwidth. Choosing `max_receive_window_size` to large is wasteful and - // delays backpressure from the receiver to the sender on the stream. - // - // In case the remote sender has exhausted half or more of its credit in less than 2 - // round-trips, try to double `max_receive_window_size`. - // - // For simplicity `max_receive_window_size` is never decreased. - // - // This implementation is heavily influenced by QUIC. See document below for rational on the - // above strategy. - // - // https://docs.google.com/document/d/1F2YfdDXKpy20WVKJueEf4abn_LVZHhMUMS5gX6Pgjl4/edit?usp=sharing - if self - .rtt - .get() - .map(|rtt| self.last_window_update.elapsed() < rtt * 2) - .unwrap_or(false) - { - let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); - - // Ideally one can just double it: - let mut new_max = shared.max_receive_window_size.saturating_mul(2); - - // Then one has to consider the configured stream limit: - new_max = cmp::min( - new_max, - self.config.max_stream_receive_window.unwrap_or(u32::MAX), - ); - - // Then one has to consider the configured connection limit: - new_max = { - let connection_limit: usize = shared.max_receive_window_size as usize + - // the overall configured conneciton limit - self.config.max_connection_receive_window - // minus the minimum amount of window guaranteed to each stream - - self.config.max_num_streams * DEFAULT_CREDIT as usize - // minus the amount of bytes beyond the minimum amount (`DEFAULT_CREDIT`) - // already allocated by this and other streams on the connection. - - *accumulated_max_stream_windows; - - cmp::min(new_max, connection_limit.try_into().unwrap_or(u32::MAX)) - }; - - // Account for the additional credit on the accumulated connection counter. - *accumulated_max_stream_windows += (new_max - shared.max_receive_window_size) as usize; - drop(accumulated_max_stream_windows); - - log::debug!( - "old window_max: {} mb, new window_max: {} mb", - shared.max_receive_window_size as f64 / 1024.0 / 1024.0, - new_max as f64 / 1024.0 / 1024.0 - ); - - shared.max_receive_window_size = new_max; - - // Recalculate `next_window_update` with the new `max_receive_window_size`. - let bytes_received = - shared.max_receive_window_size - shared.current_receive_window_size; - next_window_update = - bytes_received.saturating_sub(shared.buffer.len().try_into().unwrap_or(u32::MAX)); - } - - self.last_window_update = Instant::now(); - shared.current_receive_window_size += next_window_update; - drop(shared); - - debug_assert(); - - return Some(next_window_update); - } } /// Byte data produced by the [`futures::stream::Stream`] impl of [`Stream`]. @@ -536,20 +367,20 @@ impl AsyncWrite for Stream { log::debug!("{}/{}: can no longer write", self.conn, self.id); return Poll::Ready(Err(self.write_zero_err())); } - if shared.current_send_window_size == 0 { + if shared.current_send_window_size() == 0 { log::trace!("{}/{}: no more credit left", self.conn, self.id); shared.writer = Some(cx.waker().clone()); return Poll::Pending; } let k = std::cmp::min( - shared.current_send_window_size, + shared.current_send_window_size(), buf.len().try_into().unwrap_or(u32::MAX), ); let k = std::cmp::min( k, self.config.split_send_size.try_into().unwrap_or(u32::MAX), ); - shared.current_send_window_size = shared.current_send_window_size.saturating_sub(k); + shared.consume_send_window(k); Vec::from(&buf[..k as usize]) }; let n = body.len(); @@ -604,140 +435,35 @@ impl AsyncWrite for Stream { } } -impl Drop for Stream { - fn drop(&mut self) { - let max_receive_window_size = self.shared.lock().max_receive_window_size; - let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); - - debug_assert!( - *accumulated_max_stream_windows >= (max_receive_window_size - DEFAULT_CREDIT) as usize, - "{accumulated_max_stream_windows} {max_receive_window_size}" - ); - - *accumulated_max_stream_windows -= (max_receive_window_size - DEFAULT_CREDIT) as usize; - } -} - -#[cfg(test)] -impl Clone for Stream { - fn clone(&self) -> Self { - Self { - id: self.id.clone(), - conn: self.conn.clone(), - config: self.config.clone(), - sender: self.sender.clone(), - flag: self.flag.clone(), - shared: Arc::new(Mutex::new(self.shared.lock().clone())), - accumulated_max_stream_windows: Arc::new(Mutex::new( - self.accumulated_max_stream_windows.lock().clone(), - )), - rtt: self.rtt.clone(), - last_window_update: self.last_window_update.clone(), - } - } -} - -#[cfg(test)] -impl quickcheck::Arbitrary for Stream { - fn arbitrary(g: &mut quickcheck::Gen) -> Self { - use quickcheck::GenRange; - - let mut shared = Shared::arbitrary(g); - let config = Arc::new(Config::arbitrary(g)); - let rtt = rtt::Rtt::arbitrary(g); - - // Update `shared` to align with `config` and rtt. - shared.max_receive_window_size = if rtt.get().is_none() { - DEFAULT_CREDIT - } else { - g.gen_range( - DEFAULT_CREDIT - ..cmp::min( - config.max_stream_receive_window.unwrap_or(u32::MAX), - (DEFAULT_CREDIT as usize + config.max_connection_receive_window - - (config.max_num_streams * (DEFAULT_CREDIT as usize))) - .try_into() - .unwrap_or(u32::MAX), - ) - .saturating_add(1), - ) - }; - shared.current_receive_window_size = g.gen_range(0..shared.max_receive_window_size); - - Self { - id: StreamId::new(0), - conn: connection::Id::random(), - sender: futures::channel::mpsc::channel(0).0, - flag: Flag::arbitrary(g), - accumulated_max_stream_windows: Arc::new(Mutex::new(g.gen_range( - (shared.max_receive_window_size - DEFAULT_CREDIT) as usize - ..(config.max_connection_receive_window - - config.max_num_streams * DEFAULT_CREDIT as usize - + 1), - ))), - rtt, - last_window_update: Instant::now() - - std::time::Duration::from_secs(g.gen_range(0..(60 * 60 * 24))), - config, - shared: Arc::new(Mutex::new(shared)), - } - } -} - #[derive(Debug)] pub(crate) struct Shared { state: State, - pub(crate) current_receive_window_size: u32, - pub(crate) max_receive_window_size: u32, - pub(crate) current_send_window_size: u32, + flow_controller: FlowController, pub(crate) buffer: Chunks, pub(crate) reader: Option, pub(crate) writer: Option, } -#[cfg(test)] -impl Clone for Shared { - fn clone(&self) -> Self { - Self { - state: self.state.clone(), - current_receive_window_size: self.current_receive_window_size.clone(), - max_receive_window_size: self.max_receive_window_size.clone(), - current_send_window_size: self.current_send_window_size.clone(), - buffer: Chunks::new(), - reader: self.reader.clone(), - writer: self.writer.clone(), - } - } -} - -#[cfg(test)] -impl quickcheck::Arbitrary for Shared { - fn arbitrary(g: &mut quickcheck::Gen) -> Self { - use quickcheck::GenRange; - - let max_receive_window_size = g.gen_range(DEFAULT_CREDIT..u32::MAX); - - Self { - state: State::arbitrary(g), - current_receive_window_size: g.gen_range(0..max_receive_window_size), - max_receive_window_size, - current_send_window_size: g.gen_range(0..u32::MAX), - buffer: Chunks::new(), - reader: None, - writer: None, - } - } -} - impl Shared { - fn new(current_receive_window_size: u32, current_send_window_size: u32) -> Self { + fn new( + current_receive_window_size: u32, + current_send_window_size: u32, + + accumulated_max_stream_windows: Arc>, + rtt: Rtt, + config: Arc, + ) -> Self { Shared { state: State::Open { acknowledged: false, }, - current_receive_window_size, - max_receive_window_size: DEFAULT_CREDIT, - current_send_window_size, + flow_controller: FlowController::new( + current_receive_window_size, + current_send_window_size, + accumulated_max_stream_windows, + rtt, + config, + ), buffer: Chunks::new(), reader: None, writer: None, @@ -784,6 +510,10 @@ impl Shared { current // Return the previous stream state for informational purposes. } + pub(crate) fn next_window_update(&mut self) -> Option { + self.flow_controller.next_window_update(self.buffer.len()) + } + /// Whether we are still waiting for the remote to acknowledge this stream. pub fn is_pending_ack(&self) -> bool { matches!( @@ -793,19 +523,28 @@ impl Shared { } ) } -} -#[cfg(test)] -mod tests { - use super::*; - use quickcheck::QuickCheck; + pub(crate) fn current_send_window_size(&self) -> u32 { + self.flow_controller.current_send_window_size() + } - #[test] - fn next_window_update() { - fn property(mut stream: Stream) { - stream.next_window_update(); - } + pub(crate) fn consume_send_window(&mut self, i: u32) { + self.flow_controller.consume_send_window(i) + } + + pub(crate) fn increase_send_window_by(&mut self, i: u32) { + self.flow_controller.increase_send_window_by(i) + } + + pub(crate) fn current_receive_window_size(&self) -> u32 { + self.flow_controller.current_receive_window_size() + } + + pub(crate) fn consume_receive_window(&mut self, i: u32) { + self.flow_controller.consume_receive_window(i) + } - QuickCheck::new().quickcheck(property as fn(_)) + pub(crate) fn max_receive_window_size(&self) -> u32 { + self.flow_controller.max_receive_window_size() } } diff --git a/yamux/src/connection/stream/flow_control.rs b/yamux/src/connection/stream/flow_control.rs new file mode 100644 index 00000000..3b6820e2 --- /dev/null +++ b/yamux/src/connection/stream/flow_control.rs @@ -0,0 +1,319 @@ +use std::{cmp, sync::Arc, time::Instant}; + +use parking_lot::Mutex; + +use crate::{connection::rtt::Rtt, Config, DEFAULT_CREDIT}; + +#[derive(Debug)] +pub(crate) struct FlowController { + config: Arc, + last_window_update: Instant, + /// See [`Connection::rtt`]. + rtt: Rtt, + /// See [`Connection::accumulated_max_stream_windows`]. + accumulated_max_stream_windows: Arc>, + current_receive_window_size: u32, + max_receive_window_size: u32, + current_send_window_size: u32, +} + +impl FlowController { + pub(crate) fn new( + current_receive_window_size: u32, + current_send_window_size: u32, + accumulated_max_stream_windows: Arc>, + rtt: Rtt, + config: Arc, + ) -> Self { + Self { + current_receive_window_size, + current_send_window_size, + config, + rtt, + accumulated_max_stream_windows, + max_receive_window_size: DEFAULT_CREDIT, + last_window_update: Instant::now(), + } + } + + /// Calculate the number of additional window bytes the receiving side (local) should grant the + /// sending side (remote) via a window update message. + /// + /// Returns `None` if too small to justify a window update message. + pub(crate) fn next_window_update(&mut self, buffer_len: usize) -> Option { + self.assert_invariants(buffer_len); + + let bytes_received = self.max_receive_window_size - self.current_receive_window_size; + let mut next_window_update = + bytes_received.saturating_sub(buffer_len.try_into().unwrap_or(u32::MAX)); + + // Don't send an update in case half or more of the window is still available to the sender. + if next_window_update < self.max_receive_window_size / 2 { + return None; + } + + log::trace!( + "received {} mb in {} seconds ({} mbit/s)", + next_window_update as f64 / 1024.0 / 1024.0, + self.last_window_update.elapsed().as_secs_f64(), + next_window_update as f64 / 1024.0 / 1024.0 * 8.0 + / self.last_window_update.elapsed().as_secs_f64() + ); + + // Auto-tuning `max_receive_window_size` + // + // The ideal `max_receive_window_size` is equal to the bandwidth-delay-product (BDP), thus + // allowing the remote sender to exhaust the entire available bandwidth on a single stream. + // Choosing `max_receive_window_size` too small prevents the remote sender from exhausting + // the available bandwidth. Choosing `max_receive_window_size` to large is wasteful and + // delays backpressure from the receiver to the sender on the stream. + // + // In case the remote sender has exhausted half or more of its credit in less than 2 + // round-trips, try to double `max_receive_window_size`. + // + // For simplicity `max_receive_window_size` is never decreased. + // + // This implementation is heavily influenced by QUIC. See document below for rational on the + // above strategy. + // + // https://docs.google.com/document/d/1F2YfdDXKpy20WVKJueEf4abn_LVZHhMUMS5gX6Pgjl4/edit?usp=sharing + if self + .rtt + .get() + .map(|rtt| self.last_window_update.elapsed() < rtt * 2) + .unwrap_or(false) + { + let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); + + // Ideally one can just double it: + let mut new_max = self.max_receive_window_size.saturating_mul(2); + + // Then one has to consider the configured stream limit: + new_max = cmp::min( + new_max, + self.config.max_stream_receive_window.unwrap_or(u32::MAX), + ); + + // Then one has to consider the configured connection limit: + new_max = { + let connection_limit: usize = self.max_receive_window_size as usize + + // the overall configured conneciton limit + self.config.max_connection_receive_window + // minus the minimum amount of window guaranteed to each stream + - self.config.max_num_streams * DEFAULT_CREDIT as usize + // minus the amount of bytes beyond the minimum amount (`DEFAULT_CREDIT`) + // already allocated by this and other streams on the connection. + - *accumulated_max_stream_windows; + + cmp::min(new_max, connection_limit.try_into().unwrap_or(u32::MAX)) + }; + + // Account for the additional credit on the accumulated connection counter. + *accumulated_max_stream_windows += (new_max - self.max_receive_window_size) as usize; + drop(accumulated_max_stream_windows); + + log::debug!( + "old window_max: {} mb, new window_max: {} mb", + self.max_receive_window_size as f64 / 1024.0 / 1024.0, + new_max as f64 / 1024.0 / 1024.0 + ); + + self.max_receive_window_size = new_max; + + // Recalculate `next_window_update` with the new `max_receive_window_size`. + let bytes_received = self.max_receive_window_size - self.current_receive_window_size; + next_window_update = + bytes_received.saturating_sub(buffer_len.try_into().unwrap_or(u32::MAX)); + } + + self.last_window_update = Instant::now(); + self.current_receive_window_size += next_window_update; + + self.assert_invariants(buffer_len); + + return Some(next_window_update); + } + + fn assert_invariants(&self, buffer_len: usize) { + if !cfg!(debug_assertions) { + return; + } + + let config = &self.config; + let rtt = self.rtt.get(); + let accumulated_max_stream_windows = *self.accumulated_max_stream_windows.lock(); + + assert!( + buffer_len <= self.max_receive_window_size as usize, + "The current buffer size never exceeds the maximum stream receive window." + ); + assert!( + self.current_receive_window_size <= self.max_receive_window_size, + "The current window never exceeds the maximum." + ); + assert!( + self.max_receive_window_size <= config.max_stream_receive_window.unwrap_or(u32::MAX), + "The maximum never exceeds the configured maximum." + ); + assert!( + (self.max_receive_window_size - DEFAULT_CREDIT) as usize + <= config.max_connection_receive_window + - config.max_num_streams * DEFAULT_CREDIT as usize, + "The maximum never exceeds its maximum portion of the configured connection limit." + ); + assert!( + (self.max_receive_window_size - DEFAULT_CREDIT) as usize + <= accumulated_max_stream_windows, + "The amount by which the stream maximum exceeds DEFAULT_CREDIT is tracked in accumulated_max_stream_windows." + ); + if rtt.is_none() { + assert_eq!( + self.max_receive_window_size, DEFAULT_CREDIT, + "The maximum is only increased iff an rtt measurement is available." + ); + } + } + + pub(crate) fn current_send_window_size(&self) -> u32 { + self.current_send_window_size + } + + pub(crate) fn consume_send_window(&mut self, i: u32) { + self.current_send_window_size = self + .current_send_window_size + .checked_sub(i) + .expect("not exceed send window"); + } + + pub(crate) fn increase_send_window_by(&mut self, i: u32) { + self.current_send_window_size = self + .current_send_window_size + .checked_add(i) + .expect("send window not to exceed u32"); + } + + pub(crate) fn current_receive_window_size(&self) -> u32 { + self.current_receive_window_size + } + + pub(crate) fn consume_receive_window(&mut self, i: u32) { + self.current_receive_window_size = self + .current_receive_window_size + .checked_sub(i) + .expect("not exceed receive window"); + } + + pub(crate) fn max_receive_window_size(&self) -> u32 { + self.max_receive_window_size + } +} + +impl Drop for FlowController { + fn drop(&mut self) { + let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); + + debug_assert!( + *accumulated_max_stream_windows + >= (self.max_receive_window_size - DEFAULT_CREDIT) as usize, + "{accumulated_max_stream_windows} {}", + self.max_receive_window_size + ); + + *accumulated_max_stream_windows -= (self.max_receive_window_size - DEFAULT_CREDIT) as usize; + } +} + +#[cfg(test)] +mod tests { + use super::*; + use quickcheck::{GenRange, QuickCheck}; + + #[derive(Debug)] + struct Input { + controller: FlowController, + buffer_len: usize, + } + + #[cfg(test)] + impl Clone for Input { + fn clone(&self) -> Self { + Self { + controller: FlowController { + config: self.controller.config.clone(), + accumulated_max_stream_windows: Arc::new(Mutex::new( + self.controller + .accumulated_max_stream_windows + .lock() + .clone(), + )), + rtt: self.controller.rtt.clone(), + last_window_update: self.controller.last_window_update.clone(), + current_receive_window_size: self.controller.current_receive_window_size, + max_receive_window_size: self.controller.max_receive_window_size, + current_send_window_size: self.controller.current_send_window_size, + }, + buffer_len: self.buffer_len, + } + } + } + + impl quickcheck::Arbitrary for Input { + fn arbitrary(g: &mut quickcheck::Gen) -> Self { + let config = Arc::new(Config::arbitrary(g)); + let rtt = Rtt::arbitrary(g); + let max_receive_window_size = if rtt.get().is_none() { + DEFAULT_CREDIT + } else { + g.gen_range( + DEFAULT_CREDIT + ..std::cmp::min( + config.max_stream_receive_window.unwrap_or(u32::MAX), + (DEFAULT_CREDIT as usize + config.max_connection_receive_window + - (config.max_num_streams * (DEFAULT_CREDIT as usize))) + .try_into() + .unwrap_or(u32::MAX), + ) + .saturating_add(1), + ) + }; + let current_receive_window_size = g.gen_range(0..max_receive_window_size); + let buffer_len = g.gen_range(0..max_receive_window_size as usize); + let accumulated_max_stream_windows = Arc::new(Mutex::new(g.gen_range( + (max_receive_window_size - DEFAULT_CREDIT) as usize + ..(config.max_connection_receive_window + - config.max_num_streams * DEFAULT_CREDIT as usize + + 1), + ))); + let last_window_update = + Instant::now() - std::time::Duration::from_secs(g.gen_range(0..(60 * 60 * 24))); + let current_send_window_size = g.gen_range(0..u32::MAX); + + Self { + controller: FlowController { + accumulated_max_stream_windows, + rtt, + last_window_update, + config, + current_receive_window_size, + max_receive_window_size, + current_send_window_size, + }, + buffer_len, + } + } + } + + #[test] + fn next_window_update() { + fn property( + Input { + mut controller, + buffer_len, + }: Input, + ) { + controller.next_window_update(buffer_len); + } + + QuickCheck::new().quickcheck(property as fn(_)) + } +} From 55872bfa5e22c3a7e47a91fe84f60bc856317177 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 3 Dec 2023 19:19:58 +0100 Subject: [PATCH 22/39] Remove Config::max_stream_receive_window --- test-harness/src/lib.rs | 10 ++- yamux/src/connection/stream/flow_control.rs | 40 +++++------- yamux/src/lib.rs | 70 +++++++-------------- 3 files changed, 47 insertions(+), 73 deletions(-) diff --git a/test-harness/src/lib.rs b/test-harness/src/lib.rs index afcef3be..bc599048 100644 --- a/test-harness/src/lib.rs +++ b/test-harness/src/lib.rs @@ -447,13 +447,19 @@ pub struct TestConfig(pub Config); impl Arbitrary for TestConfig { fn arbitrary(g: &mut Gen) -> Self { + use quickcheck::GenRange; + let mut c = Config::default(); + let max_num_streams = 512; + c.set_read_after_close(Arbitrary::arbitrary(g)); + c.set_max_num_streams(max_num_streams); if bool::arbitrary(g) { - c.set_max_stream_receive_window(Some(256 * 1024 + u32::arbitrary(g) % (768 * 1024))); + c.set_max_connection_receive_window(Some(g.gen_range(max_num_streams*(yamux::DEFAULT_CREDIT as usize)..usize::MAX))); } else { - c.set_max_stream_receive_window(None); + c.set_max_connection_receive_window(None); } + TestConfig(c) } } diff --git a/yamux/src/connection/stream/flow_control.rs b/yamux/src/connection/stream/flow_control.rs index 3b6820e2..73ba898b 100644 --- a/yamux/src/connection/stream/flow_control.rs +++ b/yamux/src/connection/stream/flow_control.rs @@ -88,22 +88,16 @@ impl FlowController { // Ideally one can just double it: let mut new_max = self.max_receive_window_size.saturating_mul(2); - // Then one has to consider the configured stream limit: - new_max = cmp::min( - new_max, - self.config.max_stream_receive_window.unwrap_or(u32::MAX), - ); - - // Then one has to consider the configured connection limit: + // But one has to consider the configured connection limit: new_max = { let connection_limit: usize = self.max_receive_window_size as usize + // the overall configured conneciton limit - self.config.max_connection_receive_window + (self.config.max_connection_receive_window.unwrap_or(usize::MAX) // minus the minimum amount of window guaranteed to each stream - self.config.max_num_streams * DEFAULT_CREDIT as usize // minus the amount of bytes beyond the minimum amount (`DEFAULT_CREDIT`) // already allocated by this and other streams on the connection. - - *accumulated_max_stream_windows; + - *accumulated_max_stream_windows); cmp::min(new_max, connection_limit.try_into().unwrap_or(u32::MAX)) }; @@ -151,13 +145,9 @@ impl FlowController { self.current_receive_window_size <= self.max_receive_window_size, "The current window never exceeds the maximum." ); - assert!( - self.max_receive_window_size <= config.max_stream_receive_window.unwrap_or(u32::MAX), - "The maximum never exceeds the configured maximum." - ); assert!( (self.max_receive_window_size - DEFAULT_CREDIT) as usize - <= config.max_connection_receive_window + <= config.max_connection_receive_window.unwrap_or(usize::MAX) - config.max_num_streams * DEFAULT_CREDIT as usize, "The maximum never exceeds its maximum portion of the configured connection limit." ); @@ -261,28 +251,28 @@ mod tests { fn arbitrary(g: &mut quickcheck::Gen) -> Self { let config = Arc::new(Config::arbitrary(g)); let rtt = Rtt::arbitrary(g); + + let max_connection_minus_default = + config.max_connection_receive_window.unwrap_or(usize::MAX) + - (config.max_num_streams * (DEFAULT_CREDIT as usize)); + let max_receive_window_size = if rtt.get().is_none() { DEFAULT_CREDIT } else { g.gen_range( DEFAULT_CREDIT - ..std::cmp::min( - config.max_stream_receive_window.unwrap_or(u32::MAX), - (DEFAULT_CREDIT as usize + config.max_connection_receive_window - - (config.max_num_streams * (DEFAULT_CREDIT as usize))) - .try_into() - .unwrap_or(u32::MAX), - ) - .saturating_add(1), + ..(DEFAULT_CREDIT as usize) + .saturating_add(max_connection_minus_default) + .try_into() + .unwrap_or(u32::MAX) + .saturating_add(1), ) }; let current_receive_window_size = g.gen_range(0..max_receive_window_size); let buffer_len = g.gen_range(0..max_receive_window_size as usize); let accumulated_max_stream_windows = Arc::new(Mutex::new(g.gen_range( (max_receive_window_size - DEFAULT_CREDIT) as usize - ..(config.max_connection_receive_window - - config.max_num_streams * DEFAULT_CREDIT as usize - + 1), + ..max_connection_minus_default.saturating_add(1), ))); let last_window_update = Instant::now() - std::time::Duration::from_secs(g.gen_range(0..(60 * 60 * 24))); diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index 6d8ba28d..4d0e1461 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -68,16 +68,13 @@ const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * 1024; /// /// The default configuration values are as follows: /// -/// - receive window = 256 KiB -/// - max. buffer size (per stream) = 1 MiB -/// - max. number of streams = 8192 -/// - window update mode = on read +/// - max. sum of stream receive windows per connection = 1 GiB +/// - max. number of streams = 512 /// - read after close = true /// - split send size = 16 KiB #[derive(Debug, Clone)] pub struct Config { - max_stream_receive_window: Option, - max_connection_receive_window: usize, + max_connection_receive_window: Option, max_num_streams: usize, read_after_close: bool, split_send_size: usize, @@ -86,11 +83,7 @@ pub struct Config { impl Default for Config { fn default() -> Self { Config { - // TODO: Add rational: given that we have a connection window, ... - max_stream_receive_window: None, - // TODO: reevaluate default. - max_connection_receive_window: 1 * 1024 * 1024 * 1024, - // TODO + max_connection_receive_window: Some(1 * 1024 * 1024 * 1024), max_num_streams: 512, read_after_close: true, split_send_size: DEFAULT_SPLIT_SEND_SIZE, @@ -99,40 +92,37 @@ impl Default for Config { } impl Config { - /// Set the maximum receive window per stream . + /// Set the upper limit for the total receive window size across all streams of a connection. /// - /// Must be `>= 256 KiB`. + /// Must be `>= 256 KiB * max_num_streams` to allow each stream at least the Yamux default + /// window size. /// /// The window of a stream starts at 256 KiB and is increased (auto-tuned) based on the /// connection's round-trip time and the stream's bandwidth (striving for the /// bandwidth-delay-product). /// - /// Set to `None` to disable the per stream maximum receive window. - /// - /// # Panics - /// - /// If the given receive window is < 256 KiB. - pub fn set_max_stream_receive_window(&mut self, n: Option) -> &mut Self { - self.max_stream_receive_window = n; - self.check(); - self - } - - // TODO: Is a usize really needed here? - // TODO: Should this be an option? - /// Set the maximum sum of all stream receive windows per connection. - /// - /// Must be `>= 256 KiB * max_num_streams` to allow each stream the Yamux default window size. - pub fn set_max_connection_receive_window(&mut self, n: usize) -> &mut Self { + /// Set to `None` to disable limit, i.e. allow each stream to grow receive window based on + /// connection's round-trip time and stream's bandwidth without limit. + pub fn set_max_connection_receive_window(&mut self, n: Option) -> &mut Self { self.max_connection_receive_window = n; - self.check(); + + assert!( + self.max_connection_receive_window.unwrap_or(usize::MAX) + >= self.max_num_streams * DEFAULT_CREDIT as usize + ); + self } /// Set the max. number of streams per connection. pub fn set_max_num_streams(&mut self, n: usize) -> &mut Self { self.max_num_streams = n; - self.check(); + + assert!( + self.max_connection_receive_window.unwrap_or(usize::MAX) + >= self.max_num_streams * DEFAULT_CREDIT as usize + ); + self } @@ -140,7 +130,6 @@ impl Config { /// the connection has been closed. pub fn set_read_after_close(&mut self, b: bool) -> &mut Self { self.read_after_close = b; - self.check(); self } @@ -148,17 +137,8 @@ impl Config { /// than the configured max. will be split. pub fn set_split_send_size(&mut self, n: usize) -> &mut Self { self.split_send_size = n; - self.check(); self } - - // TODO: Consider doing the check on creation, not on each builder method call. - fn check(&self) { - assert!(self.max_stream_receive_window.unwrap_or(u32::MAX) >= DEFAULT_CREDIT); - assert!( - self.max_connection_receive_window >= self.max_num_streams * DEFAULT_CREDIT as usize - ); - } } // Check that we can safely cast a `usize` to a `u64`. @@ -179,13 +159,11 @@ impl quickcheck::Arbitrary for Config { let max_num_streams = g.gen_range(0..u16::MAX as usize); Config { - max_stream_receive_window: if bool::arbitrary(g) { - Some(g.gen_range(DEFAULT_CREDIT..u32::MAX)) + max_connection_receive_window: if bool::arbitrary(g) { + Some(g.gen_range((DEFAULT_CREDIT as usize * max_num_streams)..usize::MAX)) } else { None }, - max_connection_receive_window: g - .gen_range((DEFAULT_CREDIT as usize * max_num_streams)..usize::MAX), max_num_streams, read_after_close: bool::arbitrary(g), split_send_size: g.gen_range(DEFAULT_SPLIT_SEND_SIZE..usize::MAX), From 4d59c6d7ca37dd33ab328c360f29a68588e9f4a9 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 3 Dec 2023 19:23:51 +0100 Subject: [PATCH 23/39] Reduce diff --- test-harness/tests/poll_api.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test-harness/tests/poll_api.rs b/test-harness/tests/poll_api.rs index b90c1aaf..4d21496e 100644 --- a/test-harness/tests/poll_api.rs +++ b/test-harness/tests/poll_api.rs @@ -173,11 +173,13 @@ fn prop_config_send_recv_single() { msgs.insert(0, Msg(vec![1u8; yamux::DEFAULT_CREDIT as usize])); Runtime::new().unwrap().block_on(async move { - let (server, mut client) = connected_peers(cfg1, cfg2, None).await.unwrap(); + let (server, mut client) = connected_peers(cfg1, cfg2, None).await?; let server = echo_server(server); let client = async { - let stream = future::poll_fn(|cx| client.poll_new_outbound(cx)).await?; + let stream = future::poll_fn(|cx| client.poll_new_outbound(cx)) + .await + .unwrap(); let client_task = noop_server(stream::poll_fn(|cx| client.poll_next_inbound(cx))); future::select(pin!(client_task), pin!(send_on_single_stream(stream, msgs))).await; From 0ac553492d0e5b3b79049744a1b8b12f9aef9cb5 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 3 Dec 2023 19:30:34 +0100 Subject: [PATCH 24/39] Remove unreachable buffer length exceeded The current receive window never grows beyond the maximum. That includes the buffer size. In other words, data is only ever accounted as delivered once it left the buffer. Thus this `if` is obsolete. --- yamux/src/connection.rs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 144114a9..a7cfcba0 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -650,17 +650,6 @@ impl Active { if is_finish { shared.update_state(self.id, stream_id, State::RecvClosed); } - // TODO: Still relevant? This should include frame.body_len() no? - if shared.buffer.len() >= shared.max_receive_window_size() as usize { - log::error!( - "{}/{}: buffer of stream grows beyond limit", - self.id, - stream_id - ); - let mut header = Header::data(stream_id, 0); - header.rst(); - return Action::Reset(Frame::new(header)); - } shared.consume_receive_window(frame.body_len()); shared.buffer.push(frame.into_body()); if let Some(w) = shared.reader.take() { From c8269b6684e3ce6a471fef21990117dd3fb80fe3 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 3 Dec 2023 19:35:26 +0100 Subject: [PATCH 25/39] docs --- yamux/src/connection/stream.rs | 1 - yamux/src/lib.rs | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index ceba6f5f..4205b292 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -448,7 +448,6 @@ impl Shared { fn new( current_receive_window_size: u32, current_send_window_size: u32, - accumulated_max_stream_windows: Arc>, rtt: Rtt, config: Arc, diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index 4d0e1461..235bddca 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -63,12 +63,11 @@ const MAX_ACK_BACKLOG: usize = 256; /// https://github.com/paritytech/yamux/issues/100. const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * 1024; -// TODO: Update /// Yamux configuration. /// /// The default configuration values are as follows: /// -/// - max. sum of stream receive windows per connection = 1 GiB +/// - max. for the total receive window size across all streams of a connection = 1 GiB /// - max. number of streams = 512 /// - read after close = true /// - split send size = 16 KiB From fc6aaf7f981f12e4e8e28f968ff9d59fe99e79c2 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 3 Dec 2023 19:42:32 +0100 Subject: [PATCH 26/39] Add explainer for asserts --- yamux/src/lib.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index 235bddca..56a4cf25 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -107,7 +107,9 @@ impl Config { assert!( self.max_connection_receive_window.unwrap_or(usize::MAX) - >= self.max_num_streams * DEFAULT_CREDIT as usize + >= self.max_num_streams * DEFAULT_CREDIT as usize, + "`max_connection_receive_window` must be `>= 256 KiB * max_num_streams` to allow each + stream at least the Yamux default window size" ); self @@ -119,7 +121,9 @@ impl Config { assert!( self.max_connection_receive_window.unwrap_or(usize::MAX) - >= self.max_num_streams * DEFAULT_CREDIT as usize + >= self.max_num_streams * DEFAULT_CREDIT as usize, + "`max_connection_receive_window` must be `>= 256 KiB * max_num_streams` to allow each + stream at least the Yamux default window size" ); self From d3a22b86594cea41662ee5e6b6a067f49ee2d34b Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 3 Dec 2023 19:44:24 +0100 Subject: [PATCH 27/39] Remove obsolete code --- yamux/src/connection.rs | 6 ------ yamux/src/connection/stream.rs | 4 ---- yamux/src/connection/stream/flow_control.rs | 4 ---- 3 files changed, 14 deletions(-) diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index a7cfcba0..6892a7b5 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -315,8 +315,6 @@ pub(crate) enum Action { New(Stream), /// A ping should be answered. Ping(Frame), - /// A stream should be reset. - Reset(Frame), /// The connection should be terminated. Terminate(Frame), } @@ -569,10 +567,6 @@ impl Active { log::trace!("{}/{}: pong", self.id, f.header().stream_id()); self.pending_frames.push_back(f.into()); } - Action::Reset(f) => { - log::trace!("{}/{}: sending reset", self.id, f.header().stream_id()); - self.pending_frames.push_back(f.into()); - } Action::Terminate(f) => { log::trace!("{}: sending term", self.id); self.pending_frames.push_back(f.into()); diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 4205b292..2b749da6 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -542,8 +542,4 @@ impl Shared { pub(crate) fn consume_receive_window(&mut self, i: u32) { self.flow_controller.consume_receive_window(i) } - - pub(crate) fn max_receive_window_size(&self) -> u32 { - self.flow_controller.max_receive_window_size() - } } diff --git a/yamux/src/connection/stream/flow_control.rs b/yamux/src/connection/stream/flow_control.rs index 73ba898b..0b82cb73 100644 --- a/yamux/src/connection/stream/flow_control.rs +++ b/yamux/src/connection/stream/flow_control.rs @@ -192,10 +192,6 @@ impl FlowController { .checked_sub(i) .expect("not exceed receive window"); } - - pub(crate) fn max_receive_window_size(&self) -> u32 { - self.max_receive_window_size - } } impl Drop for FlowController { From db2971b1aafa162e8a53496d95ae88c38dd69da1 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 3 Dec 2023 19:46:29 +0100 Subject: [PATCH 28/39] fmt --- yamux/src/connection/stream/flow_control.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/yamux/src/connection/stream/flow_control.rs b/yamux/src/connection/stream/flow_control.rs index 0b82cb73..f7ad6ba7 100644 --- a/yamux/src/connection/stream/flow_control.rs +++ b/yamux/src/connection/stream/flow_control.rs @@ -152,10 +152,10 @@ impl FlowController { "The maximum never exceeds its maximum portion of the configured connection limit." ); assert!( - (self.max_receive_window_size - DEFAULT_CREDIT) as usize - <= accumulated_max_stream_windows, - "The amount by which the stream maximum exceeds DEFAULT_CREDIT is tracked in accumulated_max_stream_windows." - ); + (self.max_receive_window_size - DEFAULT_CREDIT) as usize + <= accumulated_max_stream_windows, + "The amount by which the stream maximum exceeds DEFAULT_CREDIT is tracked in accumulated_max_stream_windows." + ); if rtt.is_none() { assert_eq!( self.max_receive_window_size, DEFAULT_CREDIT, From 8d69551689c0e1131e0b458b2a74e4cf22cafcfe Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sun, 3 Dec 2023 19:51:50 +0100 Subject: [PATCH 29/39] fmt --- test-harness/src/lib.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test-harness/src/lib.rs b/test-harness/src/lib.rs index bc599048..58412ac3 100644 --- a/test-harness/src/lib.rs +++ b/test-harness/src/lib.rs @@ -455,7 +455,9 @@ impl Arbitrary for TestConfig { c.set_read_after_close(Arbitrary::arbitrary(g)); c.set_max_num_streams(max_num_streams); if bool::arbitrary(g) { - c.set_max_connection_receive_window(Some(g.gen_range(max_num_streams*(yamux::DEFAULT_CREDIT as usize)..usize::MAX))); + c.set_max_connection_receive_window(Some( + g.gen_range(max_num_streams * (yamux::DEFAULT_CREDIT as usize)..usize::MAX), + )); } else { c.set_max_connection_receive_window(None); } From 8261e12825f1445d35fc86bb2662d4857c4e4fa7 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 4 Dec 2023 12:03:46 +0100 Subject: [PATCH 30/39] Define and use KIB MIB and GIB constants --- yamux/src/connection/stream/flow_control.rs | 8 ++++---- yamux/src/frame/io.rs | 2 +- yamux/src/lib.rs | 10 +++++++--- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/yamux/src/connection/stream/flow_control.rs b/yamux/src/connection/stream/flow_control.rs index f7ad6ba7..683112b9 100644 --- a/yamux/src/connection/stream/flow_control.rs +++ b/yamux/src/connection/stream/flow_control.rs @@ -54,9 +54,9 @@ impl FlowController { log::trace!( "received {} mb in {} seconds ({} mbit/s)", - next_window_update as f64 / 1024.0 / 1024.0, + next_window_update as f64 / crate::MIB as f64, self.last_window_update.elapsed().as_secs_f64(), - next_window_update as f64 / 1024.0 / 1024.0 * 8.0 + next_window_update as f64 / crate::MIB as f64 * 8.0 / self.last_window_update.elapsed().as_secs_f64() ); @@ -108,8 +108,8 @@ impl FlowController { log::debug!( "old window_max: {} mb, new window_max: {} mb", - self.max_receive_window_size as f64 / 1024.0 / 1024.0, - new_max as f64 / 1024.0 / 1024.0 + self.max_receive_window_size as f64 / crate::MIB as f64, + new_max as f64 / crate::MIB as f64 ); self.max_receive_window_size = new_max; diff --git a/yamux/src/frame/io.rs b/yamux/src/frame/io.rs index 4290e224..3712a7c2 100644 --- a/yamux/src/frame/io.rs +++ b/yamux/src/frame/io.rs @@ -25,7 +25,7 @@ use std::{ /// Limits the amount of bytes a remote can cause the local node to allocate at once when reading. /// /// Chosen based on intuition in past iterations. -const MAX_FRAME_BODY_LEN: usize = 1024 * 1024; +const MAX_FRAME_BODY_LEN: usize = 1 * crate::MIB; /// A [`Stream`] and writer of [`Frame`] values. #[derive(Debug)] diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index 56a4cf25..d9212920 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -38,7 +38,11 @@ pub use crate::frame::{ FrameDecodeError, }; -pub const DEFAULT_CREDIT: u32 = 256 * 1024; // as per yamux specification +const KIB: usize = 1024; +const MIB: usize = kib * 1024; +const GIB: usize = mib * 1024; + +pub const DEFAULT_CREDIT: u32 = 256 * KIB as u32; // as per yamux specification pub type Result = std::result::Result; @@ -61,7 +65,7 @@ const MAX_ACK_BACKLOG: usize = 256; /// /// For details on why this concrete value was chosen, see /// https://github.com/paritytech/yamux/issues/100. -const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * 1024; +const DEFAULT_SPLIT_SEND_SIZE: usize = 16 * KIB; /// Yamux configuration. /// @@ -82,7 +86,7 @@ pub struct Config { impl Default for Config { fn default() -> Self { Config { - max_connection_receive_window: Some(1 * 1024 * 1024 * 1024), + max_connection_receive_window: Some(1 * GIB), max_num_streams: 512, read_after_close: true, split_send_size: DEFAULT_SPLIT_SEND_SIZE, From 7581f76371af45b2ffca4a3c3660e03c5cdb865d Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 4 Dec 2023 12:26:37 +0100 Subject: [PATCH 31/39] Document DOS mitigation --- yamux/src/lib.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index d9212920..4399225e 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -106,6 +106,23 @@ impl Config { /// /// Set to `None` to disable limit, i.e. allow each stream to grow receive window based on /// connection's round-trip time and stream's bandwidth without limit. + /// + /// ## DOS attack mitigation + /// + /// A remote node (attacker) might trick the local node (target) into allocating large stream + /// receive windows, trying to make the local node run out of memory. + /// + /// This attack is difficult, as the local node only increases the stream receive window up to + /// 2x the bandwidth-delay-product, where bandwidth is the amount of bytes read, not just + /// received. In other words, the attacker has to send (and have the local node read) + /// significant amount of bytes on a stream over a long period of time to increase the stream + /// receive window. E.g. on a 60ms 10Gbit/s connection the bandwidth-delay-product is ~75 MiB + /// and thus the local node will at most allocate ~150 MiB (2x bandwidth-delay-product) per + /// stream. + /// + /// Despite the difficulty of the attack one should choose a reasonable + /// `max_connection_receive_window` to protect against this attack, especially since an attacker + /// might use more than one stream per connection. pub fn set_max_connection_receive_window(&mut self, n: Option) -> &mut Self { self.max_connection_receive_window = n; From c9c71b693c4301db2273713e3a00d26a78d87fe4 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 4 Dec 2023 12:32:41 +0100 Subject: [PATCH 32/39] Document no ping on idle connection --- yamux/src/connection.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 6892a7b5..1b2b0cf7 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -384,6 +384,9 @@ impl Active { fn poll(&mut self, cx: &mut Context<'_>) -> Poll> { loop { if self.socket.poll_ready_unpin(cx).is_ready() { + // Note `next_ping` does not register a waker and thus if not called regularly (idle + // connection) no ping is sent. This is deliberate as an idle connection does not + // need RTT measurements to increase its stream receive window. if let Some(frame) = self.rtt.next_ping() { self.socket.start_send_unpin(frame.into())?; continue; From 82106b0a25e2fff6601f9a39d66e3cd1cd4756f6 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 4 Dec 2023 12:46:28 +0100 Subject: [PATCH 33/39] Derive Clone --- yamux/src/connection/rtt.rs | 26 ++------------------------ 1 file changed, 2 insertions(+), 24 deletions(-) diff --git a/yamux/src/connection/rtt.rs b/yamux/src/connection/rtt.rs index 395e465c..6dfc3a65 100644 --- a/yamux/src/connection/rtt.rs +++ b/yamux/src/connection/rtt.rs @@ -101,21 +101,12 @@ impl quickcheck::Arbitrary for Rtt { } #[derive(Debug)] +#[cfg_attr(test, derive(Clone))] struct RttInner { state: RttState, rtt: Option, } -#[cfg(test)] -impl Clone for RttInner { - fn clone(&self) -> Self { - Self { - state: self.state.clone(), - rtt: self.rtt.clone(), - } - } -} - #[cfg(test)] impl quickcheck::Arbitrary for RttInner { fn arbitrary(g: &mut quickcheck::Gen) -> Self { @@ -131,26 +122,13 @@ impl quickcheck::Arbitrary for RttInner { } #[derive(Debug)] +#[cfg_attr(test, derive(Clone))] enum RttState { Initial, AwaitingPong { sent_at: Instant, nonce: u32 }, Waiting { next: Instant }, } -#[cfg(test)] -impl Clone for RttState { - fn clone(&self) -> Self { - match self { - Self::Initial => Self::Initial, - Self::AwaitingPong { sent_at, nonce } => Self::AwaitingPong { - sent_at: sent_at.clone(), - nonce: nonce.clone(), - }, - Self::Waiting { next } => Self::Waiting { next: next.clone() }, - } - } -} - #[cfg(test)] impl quickcheck::Arbitrary for RttState { fn arbitrary(g: &mut quickcheck::Gen) -> Self { From c3f581f5b4a72d6af2edd3c24f77bde34d8f7552 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 4 Dec 2023 12:50:37 +0100 Subject: [PATCH 34/39] Remove RttState::Initial --- yamux/src/connection/rtt.rs | 22 ++++++++++------------ yamux/src/lib.rs | 4 ++-- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/yamux/src/connection/rtt.rs b/yamux/src/connection/rtt.rs index 6dfc3a65..7222c302 100644 --- a/yamux/src/connection/rtt.rs +++ b/yamux/src/connection/rtt.rs @@ -29,7 +29,9 @@ impl Rtt { pub(crate) fn new() -> Self { Self(Arc::new(Mutex::new(RttInner { rtt: None, - state: RttState::Initial, + state: RttState::Waiting { + next: Instant::now(), + }, }))) } @@ -37,7 +39,6 @@ impl Rtt { let state = &mut self.0.lock().state; match state { - RttState::Initial => {} RttState::AwaitingPong { .. } => return None, RttState::Waiting { next } => { if *next > Instant::now() { @@ -59,7 +60,7 @@ impl Rtt { let inner = &mut self.0.lock(); let (sent_at, expected_nonce) = match inner.state { - RttState::Initial | RttState::Waiting { .. } => { + RttState::Waiting { .. } => { log::error!("received unexpected pong {}", received_nonce); return Action::Terminate(Frame::protocol_error()); } @@ -124,7 +125,6 @@ impl quickcheck::Arbitrary for RttInner { #[derive(Debug)] #[cfg_attr(test, derive(Clone))] enum RttState { - Initial, AwaitingPong { sent_at: Instant, nonce: u32 }, Waiting { next: Instant }, } @@ -132,17 +132,15 @@ enum RttState { #[cfg(test)] impl quickcheck::Arbitrary for RttState { fn arbitrary(g: &mut quickcheck::Gen) -> Self { - use quickcheck::GenRange; - match g.gen_range(0u8..3) { - 0 => RttState::Initial, - 1 => RttState::AwaitingPong { + if bool::arbitrary(g) { + RttState::AwaitingPong { sent_at: Instant::now(), nonce: u32::arbitrary(g), - }, - 2 => RttState::Waiting { + } + } else { + RttState::Waiting { next: Instant::now(), - }, - _ => unreachable!(), + } } } } diff --git a/yamux/src/lib.rs b/yamux/src/lib.rs index 4399225e..c082abb0 100644 --- a/yamux/src/lib.rs +++ b/yamux/src/lib.rs @@ -39,8 +39,8 @@ pub use crate::frame::{ }; const KIB: usize = 1024; -const MIB: usize = kib * 1024; -const GIB: usize = mib * 1024; +const MIB: usize = KIB * 1024; +const GIB: usize = MIB * 1024; pub const DEFAULT_CREDIT: u32 = 256 * KIB as u32; // as per yamux specification From 33c6b05be8bfeee657e32f6bd5bf111b2aed83fa Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 4 Dec 2023 12:53:12 +0100 Subject: [PATCH 35/39] Use `Duration`'s `fmt::Debug` implementation --- yamux/src/connection/rtt.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/yamux/src/connection/rtt.rs b/yamux/src/connection/rtt.rs index 7222c302..be734f7f 100644 --- a/yamux/src/connection/rtt.rs +++ b/yamux/src/connection/rtt.rs @@ -78,8 +78,8 @@ impl Rtt { inner.rtt = Some(sent_at.elapsed()); log::debug!( - "received pong {received_nonce}, estimated round-trip-time {}s", - inner.rtt.as_ref().unwrap().as_secs_f64() + "received pong {received_nonce}, estimated round-trip-time {:?}", + inner.rtt.as_ref().expect("rtt just set") ); inner.state = RttState::Waiting { From 68e5e1ccbf8c762094b304cd15937576a5237127 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 4 Dec 2023 12:55:15 +0100 Subject: [PATCH 36/39] Refactor string formatting --- yamux/src/connection/rtt.rs | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/yamux/src/connection/rtt.rs b/yamux/src/connection/rtt.rs index be734f7f..56938d7f 100644 --- a/yamux/src/connection/rtt.rs +++ b/yamux/src/connection/rtt.rs @@ -61,26 +61,20 @@ impl Rtt { let (sent_at, expected_nonce) = match inner.state { RttState::Waiting { .. } => { - log::error!("received unexpected pong {}", received_nonce); + log::error!("received unexpected pong {received_nonce}"); return Action::Terminate(Frame::protocol_error()); } RttState::AwaitingPong { sent_at, nonce } => (sent_at, nonce), }; if received_nonce != expected_nonce { - log::error!( - "received pong with {} but expected {}", - received_nonce, - expected_nonce - ); + log::error!("received pong with {received_nonce} but expected {expected_nonce}"); return Action::Terminate(Frame::protocol_error()); } - inner.rtt = Some(sent_at.elapsed()); - log::debug!( - "received pong {received_nonce}, estimated round-trip-time {:?}", - inner.rtt.as_ref().expect("rtt just set") - ); + let rtt = sent_at.elapsed(); + inner.rtt = Some(rtt); + log::debug!("received pong {received_nonce}, estimated round-trip-time {rtt:?}"); inner.state = RttState::Waiting { next: Instant::now() + PING_INTERVAL, From aec5fd52b90a3ddc7635f805e48bd6a0cba63352 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 4 Dec 2023 13:05:25 +0100 Subject: [PATCH 37/39] Add changelog entry --- CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index afd00caa..295cf9df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ # 0.13.0 +- Introduce dynamic stream receive window auto-tuning. + While low-resourced deployments maintain the benefit of small buffers, high resource deployments eventually end-up with a window of roughly the bandwidth-delay-product (ideal) and are thus able to use the entire available bandwidth. + See [PR 176](https://github.com/libp2p/rust-yamux/pull/176) for performance results and details on the implementation. - Remove `WindowUpdateMode`. Behavior will always be `WindowUpdateMode::OnRead`, thus enabling flow-control and enforcing backpressure. See [PR 178](https://github.com/libp2p/rust-yamux/pull/178). From 79c0d11e8b08ead4f7c39ee6cec7d2cfe36686be Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 4 Dec 2023 13:13:40 +0100 Subject: [PATCH 38/39] Use shadowing over mut --- yamux/src/connection/stream/flow_control.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/yamux/src/connection/stream/flow_control.rs b/yamux/src/connection/stream/flow_control.rs index 683112b9..77c528c2 100644 --- a/yamux/src/connection/stream/flow_control.rs +++ b/yamux/src/connection/stream/flow_control.rs @@ -86,10 +86,10 @@ impl FlowController { let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); // Ideally one can just double it: - let mut new_max = self.max_receive_window_size.saturating_mul(2); + let new_max = self.max_receive_window_size.saturating_mul(2); // But one has to consider the configured connection limit: - new_max = { + let new_max = { let connection_limit: usize = self.max_receive_window_size as usize + // the overall configured conneciton limit (self.config.max_connection_receive_window.unwrap_or(usize::MAX) From 6b64a9fad14b6744e817e0fbc92f2c6126461f71 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Mon, 4 Dec 2023 13:22:18 +0100 Subject: [PATCH 39/39] Remove current_ and _size --- yamux/src/connection.rs | 2 +- yamux/src/connection/stream.rs | 24 ++--- yamux/src/connection/stream/flow_control.rs | 109 ++++++++++---------- 3 files changed, 67 insertions(+), 68 deletions(-) diff --git a/yamux/src/connection.rs b/yamux/src/connection.rs index 1b2b0cf7..27beb28e 100644 --- a/yamux/src/connection.rs +++ b/yamux/src/connection.rs @@ -636,7 +636,7 @@ impl Active { if let Some(s) = self.streams.get_mut(&stream_id) { let mut shared = s.lock(); - if frame.body_len() > shared.current_receive_window_size() { + if frame.body_len() > shared.receive_window() { log::error!( "{}/{}: frame body larger than window of stream", self.id, diff --git a/yamux/src/connection/stream.rs b/yamux/src/connection/stream.rs index 2b749da6..1f48e1b9 100644 --- a/yamux/src/connection/stream.rs +++ b/yamux/src/connection/stream.rs @@ -119,7 +119,7 @@ impl Stream { id: StreamId, conn: connection::Id, config: Arc, - current_send_window_size: u32, + send_window: u32, sender: mpsc::Sender, rtt: rtt::Rtt, accumulated_max_stream_windows: Arc>, @@ -132,7 +132,7 @@ impl Stream { flag: Flag::Ack, shared: Arc::new(Mutex::new(Shared::new( DEFAULT_CREDIT, - current_send_window_size, + send_window, accumulated_max_stream_windows, rtt, config, @@ -367,13 +367,13 @@ impl AsyncWrite for Stream { log::debug!("{}/{}: can no longer write", self.conn, self.id); return Poll::Ready(Err(self.write_zero_err())); } - if shared.current_send_window_size() == 0 { + if shared.send_window() == 0 { log::trace!("{}/{}: no more credit left", self.conn, self.id); shared.writer = Some(cx.waker().clone()); return Poll::Pending; } let k = std::cmp::min( - shared.current_send_window_size(), + shared.send_window(), buf.len().try_into().unwrap_or(u32::MAX), ); let k = std::cmp::min( @@ -446,8 +446,8 @@ pub(crate) struct Shared { impl Shared { fn new( - current_receive_window_size: u32, - current_send_window_size: u32, + receive_window: u32, + send_window: u32, accumulated_max_stream_windows: Arc>, rtt: Rtt, config: Arc, @@ -457,8 +457,8 @@ impl Shared { acknowledged: false, }, flow_controller: FlowController::new( - current_receive_window_size, - current_send_window_size, + receive_window, + send_window, accumulated_max_stream_windows, rtt, config, @@ -523,8 +523,8 @@ impl Shared { ) } - pub(crate) fn current_send_window_size(&self) -> u32 { - self.flow_controller.current_send_window_size() + pub(crate) fn send_window(&self) -> u32 { + self.flow_controller.send_window() } pub(crate) fn consume_send_window(&mut self, i: u32) { @@ -535,8 +535,8 @@ impl Shared { self.flow_controller.increase_send_window_by(i) } - pub(crate) fn current_receive_window_size(&self) -> u32 { - self.flow_controller.current_receive_window_size() + pub(crate) fn receive_window(&self) -> u32 { + self.flow_controller.receive_window() } pub(crate) fn consume_receive_window(&mut self, i: u32) { diff --git a/yamux/src/connection/stream/flow_control.rs b/yamux/src/connection/stream/flow_control.rs index 77c528c2..9d9bd6cf 100644 --- a/yamux/src/connection/stream/flow_control.rs +++ b/yamux/src/connection/stream/flow_control.rs @@ -12,26 +12,26 @@ pub(crate) struct FlowController { rtt: Rtt, /// See [`Connection::accumulated_max_stream_windows`]. accumulated_max_stream_windows: Arc>, - current_receive_window_size: u32, - max_receive_window_size: u32, - current_send_window_size: u32, + receive_window: u32, + max_receive_window: u32, + send_window: u32, } impl FlowController { pub(crate) fn new( - current_receive_window_size: u32, - current_send_window_size: u32, + receive_window: u32, + send_window: u32, accumulated_max_stream_windows: Arc>, rtt: Rtt, config: Arc, ) -> Self { Self { - current_receive_window_size, - current_send_window_size, + receive_window, + send_window, config, rtt, accumulated_max_stream_windows, - max_receive_window_size: DEFAULT_CREDIT, + max_receive_window: DEFAULT_CREDIT, last_window_update: Instant::now(), } } @@ -43,12 +43,12 @@ impl FlowController { pub(crate) fn next_window_update(&mut self, buffer_len: usize) -> Option { self.assert_invariants(buffer_len); - let bytes_received = self.max_receive_window_size - self.current_receive_window_size; + let bytes_received = self.max_receive_window - self.receive_window; let mut next_window_update = bytes_received.saturating_sub(buffer_len.try_into().unwrap_or(u32::MAX)); // Don't send an update in case half or more of the window is still available to the sender. - if next_window_update < self.max_receive_window_size / 2 { + if next_window_update < self.max_receive_window / 2 { return None; } @@ -60,18 +60,18 @@ impl FlowController { / self.last_window_update.elapsed().as_secs_f64() ); - // Auto-tuning `max_receive_window_size` + // Auto-tuning `max_receive_window` // - // The ideal `max_receive_window_size` is equal to the bandwidth-delay-product (BDP), thus + // The ideal `max_receive_window` is equal to the bandwidth-delay-product (BDP), thus // allowing the remote sender to exhaust the entire available bandwidth on a single stream. - // Choosing `max_receive_window_size` too small prevents the remote sender from exhausting - // the available bandwidth. Choosing `max_receive_window_size` to large is wasteful and - // delays backpressure from the receiver to the sender on the stream. + // Choosing `max_receive_window` too small prevents the remote sender from exhausting the + // available bandwidth. Choosing `max_receive_window` to large is wasteful and delays + // backpressure from the receiver to the sender on the stream. // // In case the remote sender has exhausted half or more of its credit in less than 2 - // round-trips, try to double `max_receive_window_size`. + // round-trips, try to double `max_receive_window`. // - // For simplicity `max_receive_window_size` is never decreased. + // For simplicity `max_receive_window` is never decreased. // // This implementation is heavily influenced by QUIC. See document below for rational on the // above strategy. @@ -86,11 +86,11 @@ impl FlowController { let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); // Ideally one can just double it: - let new_max = self.max_receive_window_size.saturating_mul(2); + let new_max = self.max_receive_window.saturating_mul(2); // But one has to consider the configured connection limit: let new_max = { - let connection_limit: usize = self.max_receive_window_size as usize + + let connection_limit: usize = self.max_receive_window as usize + // the overall configured conneciton limit (self.config.max_connection_receive_window.unwrap_or(usize::MAX) // minus the minimum amount of window guaranteed to each stream @@ -103,25 +103,25 @@ impl FlowController { }; // Account for the additional credit on the accumulated connection counter. - *accumulated_max_stream_windows += (new_max - self.max_receive_window_size) as usize; + *accumulated_max_stream_windows += (new_max - self.max_receive_window) as usize; drop(accumulated_max_stream_windows); log::debug!( "old window_max: {} mb, new window_max: {} mb", - self.max_receive_window_size as f64 / crate::MIB as f64, + self.max_receive_window as f64 / crate::MIB as f64, new_max as f64 / crate::MIB as f64 ); - self.max_receive_window_size = new_max; + self.max_receive_window = new_max; - // Recalculate `next_window_update` with the new `max_receive_window_size`. - let bytes_received = self.max_receive_window_size - self.current_receive_window_size; + // Recalculate `next_window_update` with the new `max_receive_window`. + let bytes_received = self.max_receive_window - self.receive_window; next_window_update = bytes_received.saturating_sub(buffer_len.try_into().unwrap_or(u32::MAX)); } self.last_window_update = Instant::now(); - self.current_receive_window_size += next_window_update; + self.receive_window += next_window_update; self.assert_invariants(buffer_len); @@ -138,57 +138,57 @@ impl FlowController { let accumulated_max_stream_windows = *self.accumulated_max_stream_windows.lock(); assert!( - buffer_len <= self.max_receive_window_size as usize, + buffer_len <= self.max_receive_window as usize, "The current buffer size never exceeds the maximum stream receive window." ); assert!( - self.current_receive_window_size <= self.max_receive_window_size, + self.receive_window <= self.max_receive_window, "The current window never exceeds the maximum." ); assert!( - (self.max_receive_window_size - DEFAULT_CREDIT) as usize + (self.max_receive_window - DEFAULT_CREDIT) as usize <= config.max_connection_receive_window.unwrap_or(usize::MAX) - config.max_num_streams * DEFAULT_CREDIT as usize, "The maximum never exceeds its maximum portion of the configured connection limit." ); assert!( - (self.max_receive_window_size - DEFAULT_CREDIT) as usize + (self.max_receive_window - DEFAULT_CREDIT) as usize <= accumulated_max_stream_windows, "The amount by which the stream maximum exceeds DEFAULT_CREDIT is tracked in accumulated_max_stream_windows." ); if rtt.is_none() { assert_eq!( - self.max_receive_window_size, DEFAULT_CREDIT, + self.max_receive_window, DEFAULT_CREDIT, "The maximum is only increased iff an rtt measurement is available." ); } } - pub(crate) fn current_send_window_size(&self) -> u32 { - self.current_send_window_size + pub(crate) fn send_window(&self) -> u32 { + self.send_window } pub(crate) fn consume_send_window(&mut self, i: u32) { - self.current_send_window_size = self - .current_send_window_size + self.send_window = self + .send_window .checked_sub(i) .expect("not exceed send window"); } pub(crate) fn increase_send_window_by(&mut self, i: u32) { - self.current_send_window_size = self - .current_send_window_size + self.send_window = self + .send_window .checked_add(i) .expect("send window not to exceed u32"); } - pub(crate) fn current_receive_window_size(&self) -> u32 { - self.current_receive_window_size + pub(crate) fn receive_window(&self) -> u32 { + self.receive_window } pub(crate) fn consume_receive_window(&mut self, i: u32) { - self.current_receive_window_size = self - .current_receive_window_size + self.receive_window = self + .receive_window .checked_sub(i) .expect("not exceed receive window"); } @@ -199,13 +199,12 @@ impl Drop for FlowController { let mut accumulated_max_stream_windows = self.accumulated_max_stream_windows.lock(); debug_assert!( - *accumulated_max_stream_windows - >= (self.max_receive_window_size - DEFAULT_CREDIT) as usize, + *accumulated_max_stream_windows >= (self.max_receive_window - DEFAULT_CREDIT) as usize, "{accumulated_max_stream_windows} {}", - self.max_receive_window_size + self.max_receive_window ); - *accumulated_max_stream_windows -= (self.max_receive_window_size - DEFAULT_CREDIT) as usize; + *accumulated_max_stream_windows -= (self.max_receive_window - DEFAULT_CREDIT) as usize; } } @@ -234,9 +233,9 @@ mod tests { )), rtt: self.controller.rtt.clone(), last_window_update: self.controller.last_window_update.clone(), - current_receive_window_size: self.controller.current_receive_window_size, - max_receive_window_size: self.controller.max_receive_window_size, - current_send_window_size: self.controller.current_send_window_size, + receive_window: self.controller.receive_window, + max_receive_window: self.controller.max_receive_window, + send_window: self.controller.send_window, }, buffer_len: self.buffer_len, } @@ -252,7 +251,7 @@ mod tests { config.max_connection_receive_window.unwrap_or(usize::MAX) - (config.max_num_streams * (DEFAULT_CREDIT as usize)); - let max_receive_window_size = if rtt.get().is_none() { + let max_receive_window = if rtt.get().is_none() { DEFAULT_CREDIT } else { g.gen_range( @@ -264,15 +263,15 @@ mod tests { .saturating_add(1), ) }; - let current_receive_window_size = g.gen_range(0..max_receive_window_size); - let buffer_len = g.gen_range(0..max_receive_window_size as usize); + let receive_window = g.gen_range(0..max_receive_window); + let buffer_len = g.gen_range(0..max_receive_window as usize); let accumulated_max_stream_windows = Arc::new(Mutex::new(g.gen_range( - (max_receive_window_size - DEFAULT_CREDIT) as usize + (max_receive_window - DEFAULT_CREDIT) as usize ..max_connection_minus_default.saturating_add(1), ))); let last_window_update = Instant::now() - std::time::Duration::from_secs(g.gen_range(0..(60 * 60 * 24))); - let current_send_window_size = g.gen_range(0..u32::MAX); + let send_window = g.gen_range(0..u32::MAX); Self { controller: FlowController { @@ -280,9 +279,9 @@ mod tests { rtt, last_window_update, config, - current_receive_window_size, - max_receive_window_size, - current_send_window_size, + receive_window, + max_receive_window, + send_window, }, buffer_len, }