From 73a62a2a2cf73de10cb1730849c39426ed8831f1 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Mon, 18 Mar 2024 14:50:43 +0100 Subject: [PATCH 01/25] Start implementing flow-control --- crates/fluke/src/h2/body.rs | 7 ++-- crates/fluke/src/h2/server.rs | 65 +++++++++++++++++++---------------- crates/fluke/src/h2/types.rs | 34 ++++++++++++++---- 3 files changed, 68 insertions(+), 38 deletions(-) diff --git a/crates/fluke/src/h2/body.rs b/crates/fluke/src/h2/body.rs index adbafc91..f1ed8c99 100644 --- a/crates/fluke/src/h2/body.rs +++ b/crates/fluke/src/h2/body.rs @@ -8,16 +8,17 @@ pub(crate) enum PieceOrTrailers { Trailers(Box), } -pub(crate) type H2BodySender = mpsc::Sender; +pub(crate) type StreamIncoming = mpsc::Sender; + // FIXME: don't use eyre, do proper error handling -pub(crate) type H2BodyItem = eyre::Result; +pub(crate) type StreamIncomingItem = eyre::Result; #[derive(Debug)] pub(crate) struct H2Body { pub(crate) content_length: Option, pub(crate) eof: bool, // TODO: more specific error handling - pub(crate) rx: mpsc::Receiver, + pub(crate) rx: mpsc::Receiver, } impl Body for H2Body { diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index 167b17aa..c8487145 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -23,7 +23,7 @@ use tracing::{debug, trace}; use crate::{ h2::{ - body::{H2Body, H2BodyItem, PieceOrTrailers}, + body::{H2Body, PieceOrTrailers, StreamIncomingItem}, encode::{EncoderState, H2Encoder}, parse::{ self, parse_reserved_and_u31, ContinuationFlags, DataFlags, Frame, FrameType, @@ -31,7 +31,7 @@ use crate::{ }, types::{ ConnState, H2ConnectionError, H2Event, H2EventPayload, H2StreamError, - HeadersOrTrailers, StreamState, + HeadersOrTrailers, StreamOutgoing, StreamState, }, }, util::read_and_parse, @@ -428,18 +428,12 @@ impl ServerContext { // if the stream is open, this transitions to HalfClosedLocal. if let Some(ss) = self.state.streams.get_mut(&frame.stream_id) { match ss { - StreamState::Open(_) => { - // transition through StreamState::HalfClosedRemote - // so we don't have to remove/re-insert. - let mut entry = StreamState::HalfClosedRemote; - std::mem::swap(&mut entry, ss); - - let body_tx = match entry { - StreamState::Open(body_tx) => body_tx, + StreamState::Open { .. } => { + let incoming = match std::mem::take(ss) { + StreamState::Open { incoming, .. } => incoming, _ => unreachable!(), }; - - *ss = StreamState::HalfClosedLocal(body_tx); + *ss = StreamState::HalfClosedLocal { incoming }; } _ => { // transition to closed @@ -506,8 +500,9 @@ impl ServerContext { )?; match ss { - StreamState::Open(body_tx) | StreamState::HalfClosedLocal(body_tx) => { - if body_tx + StreamState::Open { incoming, .. } + | StreamState::HalfClosedLocal { incoming } => { + if incoming .send(Ok(PieceOrTrailers::Piece(payload.into()))) .await .is_err() @@ -516,10 +511,12 @@ impl ServerContext { } if flags.contains(DataFlags::EndStream) { - // if we're HalfClosedLocal, this transitions to Closed - // otherwise, it transitions to HalfClosedRemote - if matches!(ss, StreamState::Open(_)) { - *ss = StreamState::HalfClosedRemote; + if let StreamState::Open { .. } = ss { + let outgoing = match std::mem::take(ss) { + StreamState::Open { outgoing, .. } => outgoing, + _ => unreachable!(), + }; + *ss = StreamState::HalfClosedRemote { outgoing }; } else if self.state.streams.remove(&frame.stream_id).is_some() { debug!( "Closed stream (read data w/EndStream) {}, now have {} streams", @@ -529,7 +526,7 @@ impl ServerContext { } } } - StreamState::HalfClosedRemote => { + StreamState::HalfClosedRemote { .. } => { debug!( stream_id = %frame.stream_id, "Received data for closed stream" @@ -537,6 +534,7 @@ impl ServerContext { self.rst(frame.stream_id, H2StreamError::StreamClosed) .await?; } + StreamState::Transition => unreachable!(), } } FrameType::Headers(flags) => { @@ -610,7 +608,7 @@ impl ServerContext { } } } - Some(StreamState::Open(_) | StreamState::HalfClosedLocal(_)) => { + Some(StreamState::Open { .. } | StreamState::HalfClosedLocal { .. }) => { headers_or_trailers = HeadersOrTrailers::Trailers; debug!("Receiving trailers for stream {}", frame.stream_id); @@ -625,11 +623,12 @@ impl ServerContext { .await?; } } - Some(StreamState::HalfClosedRemote) => { + Some(StreamState::HalfClosedRemote { .. }) => { return Err(H2ConnectionError::StreamClosed { stream_id: frame.stream_id, }); } + Some(StreamState::Transition) => unreachable!(), } self.read_headers( @@ -693,14 +692,16 @@ impl ServerContext { self.state.streams.len() ); match ss { - StreamState::Open(body_tx) | StreamState::HalfClosedLocal(body_tx) => { - _ = body_tx + StreamState::Open { incoming, .. } + | StreamState::HalfClosedLocal { incoming, .. } => { + _ = incoming .send(Err(H2StreamError::ReceivedRstStream.into())) .await; } - StreamState::HalfClosedRemote => { + StreamState::HalfClosedRemote { .. } => { // good } + StreamState::Transition => unreachable!(), } } } @@ -1065,7 +1066,7 @@ impl ServerContext { state: ExpectResponseHeaders, }; - let (piece_tx, piece_rx) = mpsc::channel::(1); // TODO: is 1 a sensible value here? + let (piece_tx, piece_rx) = mpsc::channel::(1); // TODO: is 1 a sensible value here? let req_body = H2Body { // FIXME: that's not right. h2 requests can still specify @@ -1075,12 +1076,18 @@ impl ServerContext { rx: piece_rx, }; + let incoming = piece_tx; + let outgoing: StreamOutgoing = Default::default(); + self.state.streams.insert( stream_id, if end_stream { - StreamState::HalfClosedRemote + StreamState::HalfClosedRemote { outgoing } } else { - StreamState::Open(piece_tx) + StreamState::Open { + incoming, + outgoing: Default::default(), + } }, ); debug!( @@ -1108,8 +1115,8 @@ impl ServerContext { } HeadersOrTrailers::Trailers => { match self.state.streams.get_mut(&stream_id) { - Some(StreamState::Open(body_tx)) => { - if body_tx + Some(StreamState::Open { incoming, .. }) => { + if incoming .send(Ok(PieceOrTrailers::Trailers(Box::new(headers)))) .await .is_err() diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index a81ce148..406d7ede 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -5,7 +5,7 @@ use fluke_buffet::Piece; use crate::Response; use super::{ - body::H2BodySender, + body::StreamIncoming, parse::{FrameType, KnownErrorCode, Settings, StreamId}, }; @@ -67,19 +67,41 @@ impl Default for ConnState { // R: RST_STREAM frame // PP: PUSH_PROMISE frame (with implied CONTINUATION frames); state // transitions are for the promised stream +#[derive(Default)] pub(crate) enum StreamState { // we have received full HEADERS - Open(H2BodySender), + Open { + incoming: StreamIncoming, + outgoing: StreamOutgoing, + }, + + // the peer has sent END_STREAM/RST_STREAM (but we might still send data to the peer) + HalfClosedRemote { + outgoing: StreamOutgoing, + }, - // the peer has sent END_STREAM/RST_STREAM - HalfClosedRemote, + // we have sent END_STREAM/RST_STREAM (but we might still receive data from the peer) + HalfClosedLocal { + incoming: StreamIncoming, + }, - // we have sent END_STREAM/RST_STREAM - HalfClosedLocal(H2BodySender), + // A transition state used for state machine code + #[default] + Transition, + // // // Note: the "Closed" state is indicated by not having an entry in the map } +#[derive(Default)] +pub(crate) struct StreamOutgoing { + // list of pieces we need to send out + pieces: Vec, + + // offset within the first piece + offset: usize, +} + #[derive(Debug, thiserror::Error)] pub(crate) enum H2ConnectionError { #[error("frame too large: {frame_type:?} frame of size {frame_size} exceeds max frame size of {max_frame_size}")] From 16a769270552ef9b3f92024644e866a008731a4e Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Mon, 18 Mar 2024 16:11:33 +0100 Subject: [PATCH 02/25] Introduce StreamIncoming, StreamOutgoing --- crates/fluke/src/h2/server.rs | 37 +++++++++++++-- crates/fluke/src/h2/types.rs | 88 +++++++++++++++++++++++++++++++++-- 2 files changed, 115 insertions(+), 10 deletions(-) diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index c8487145..9f0abcb7 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -70,6 +70,7 @@ pub async fn serve( pub(crate) struct ServerContext { driver: Rc, state: ConnState, + hpack_dec: fluke_hpack::Decoder<'static>, hpack_enc: fluke_hpack::Encoder<'static>, out_scratch: RollMut, @@ -394,10 +395,30 @@ impl ServerContext { self.write_frame(frame, payload).await?; } H2EventPayload::BodyChunk(chunk) => { - let flags = BitFlags::::default(); - let frame = Frame::new(FrameType::Data(flags), ev.stream_id); + let stream = match self.state.streams.get_mut(&ev.stream_id) { + None => { + // ignore the event then, but at this point we should + // tell the sender to stop sending chunks, which is not + // possible if they all share the same ev_tx + // TODO: make it possible to propagate errors to the sender + return Ok(()); + } + Some(stream) => stream, + }; + + if let Some(outgoing) = stream.outgoing_mut() { + outgoing.pieces.push(chunk); + self.state.streams_with_pending_data.insert(ev.stream_id); + if self.state.outgoing_capacity > 0 && outgoing.capacity > 0 { + // worth revisiting then! + self.state.send_data_maybe.notify_one(); + } + } + + // let flags = BitFlags::::default(); + // let frame = Frame::new(FrameType::Data(flags), ev.stream_id); - self.write_frame(frame, chunk).await?; + // self.write_frame(frame, chunk).await?; } H2EventPayload::BodyEnd => { // FIXME: this should transition the stream to `Closed` @@ -1077,7 +1098,7 @@ impl ServerContext { }; let incoming = piece_tx; - let outgoing: StreamOutgoing = Default::default(); + let outgoing: StreamOutgoing = self.state.mk_stream_outgoing(); self.state.streams.insert( stream_id, @@ -1086,7 +1107,7 @@ impl ServerContext { } else { StreamState::Open { incoming, - outgoing: Default::default(), + outgoing: self.state.mk_stream_outgoing(), } }, ); @@ -1095,6 +1116,12 @@ impl ServerContext { self.state.streams.len() ); + // FIXME: don't spawn, just add to an unordered futures + // instead and poll it in our main loop, to do intra-task + // concurrency. + // + // this lets us freeze the entire http2 server and explore + // its entire state. fluke_maybe_uring::spawn({ let driver = self.driver.clone(); async move { diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index 406d7ede..d8e4dfdb 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -1,6 +1,10 @@ -use std::{collections::HashMap, fmt}; +use std::{ + collections::{HashMap, HashSet}, + fmt, +}; use fluke_buffet::Piece; +use tokio::sync::Notify; use crate::Response; @@ -12,17 +16,50 @@ use super::{ pub(crate) struct ConnState { pub(crate) streams: HashMap, pub(crate) last_stream_id: StreamId, + pub(crate) self_settings: Settings, pub(crate) peer_settings: Settings, + + /// notified when we have data to send, like when: + /// - an H2Body has been written to, AND + /// - the corresponding stream has available capacity + /// - the connection has available capacity + pub(crate) send_data_maybe: Notify, + pub(crate) streams_with_pending_data: HashSet, + + pub(crate) incoming_capacity: u32, + pub(crate) outgoing_capacity: u32, } impl Default for ConnState { fn default() -> Self { - Self { + let mut s = Self { streams: Default::default(), last_stream_id: StreamId(0), + self_settings: Default::default(), peer_settings: Default::default(), + + send_data_maybe: Default::default(), + streams_with_pending_data: Default::default(), + + incoming_capacity: 0, + outgoing_capacity: 0, + }; + s.incoming_capacity = s.self_settings.initial_window_size; + s.outgoing_capacity = s.peer_settings.initial_window_size; + + s + } +} + +impl ConnState { + /// create a new [StreamOutgoing] based on our current settings + pub(crate) fn mk_stream_outgoing(&self) -> StreamOutgoing { + StreamOutgoing { + pieces: Vec::new(), + offset: 0, + capacity: self.self_settings.initial_window_size, } } } @@ -93,13 +130,54 @@ pub(crate) enum StreamState { // Note: the "Closed" state is indicated by not having an entry in the map } -#[derive(Default)] +impl StreamState { + /// Get the inner `StreamIncoming` if the state is `Open` or `HalfClosedLocal`. + pub(crate) fn incoming_ref(&self) -> Option<&StreamIncoming> { + match self { + StreamState::Open { incoming, .. } => Some(incoming), + StreamState::HalfClosedLocal { incoming, .. } => Some(incoming), + _ => None, + } + } + + /// Get the inner `StreamIncoming` if the state is `Open` or `HalfClosedLocal`. + pub(crate) fn incoming_mut(&mut self) -> Option<&mut StreamIncoming> { + match self { + StreamState::Open { incoming, .. } => Some(incoming), + StreamState::HalfClosedLocal { incoming, .. } => Some(incoming), + _ => None, + } + } + + /// Get the inner `StreamOutgoing` if the state is `Open` or `HalfClosedRemote`. + pub(crate) fn outgoing_ref(&self) -> Option<&StreamOutgoing> { + match self { + StreamState::Open { outgoing, .. } => Some(outgoing), + StreamState::HalfClosedRemote { outgoing, .. } => Some(outgoing), + _ => None, + } + } + + /// Get the inner `StreamOutgoing` if the state is `Open` or `HalfClosedRemote`. + pub(crate) fn outgoing_mut(&mut self) -> Option<&mut StreamOutgoing> { + match self { + StreamState::Open { outgoing, .. } => Some(outgoing), + StreamState::HalfClosedRemote { outgoing, .. } => Some(outgoing), + _ => None, + } + } +} + pub(crate) struct StreamOutgoing { // list of pieces we need to send out - pieces: Vec, + pub(crate) pieces: Vec, // offset within the first piece - offset: usize, + pub(crate) offset: usize, + + // window size of the stream, ie. how many bytes + // we can send to the receiver before waiting. + pub(crate) capacity: u32, } #[derive(Debug, thiserror::Error)] From 5ad35a5d803a1beb85f17e144889c64ae50349a8 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Mon, 18 Mar 2024 16:35:54 +0100 Subject: [PATCH 03/25] Pass h2spec 6.9 tests apparently, even though we're never sending data now? --- crates/fluke/src/h2/body.rs | 9 +++++- crates/fluke/src/h2/server.rs | 53 ++++++++++++++++++++++++++++++----- crates/fluke/src/h2/types.rs | 17 +++++++++-- 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/crates/fluke/src/h2/body.rs b/crates/fluke/src/h2/body.rs index f1ed8c99..2ac7d52d 100644 --- a/crates/fluke/src/h2/body.rs +++ b/crates/fluke/src/h2/body.rs @@ -8,7 +8,14 @@ pub(crate) enum PieceOrTrailers { Trailers(Box), } -pub(crate) type StreamIncoming = mpsc::Sender; +pub(crate) struct StreamIncoming { + // TODO: don't allow access to tx, check against capacity first? + pub(crate) tx: mpsc::Sender, + + // incoming capacity (that we decide, we get to tell + // the peer how much we can handle with window updates) + pub(crate) capacity: u32, +} // FIXME: don't use eyre, do proper error handling pub(crate) type StreamIncomingItem = eyre::Result; diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index 9f0abcb7..b505d048 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -23,7 +23,7 @@ use tracing::{debug, trace}; use crate::{ h2::{ - body::{H2Body, PieceOrTrailers, StreamIncomingItem}, + body::{H2Body, PieceOrTrailers, StreamIncoming, StreamIncomingItem}, encode::{EncoderState, H2Encoder}, parse::{ self, parse_reserved_and_u31, ContinuationFlags, DataFlags, Frame, FrameType, @@ -524,6 +524,7 @@ impl ServerContext { StreamState::Open { incoming, .. } | StreamState::HalfClosedLocal { incoming } => { if incoming + .tx .send(Ok(PieceOrTrailers::Piece(payload.into()))) .await .is_err() @@ -716,6 +717,7 @@ impl ServerContext { StreamState::Open { incoming, .. } | StreamState::HalfClosedLocal { incoming, .. } => { _ = incoming + .tx .send(Err(H2StreamError::ReceivedRstStream.into())) .await; } @@ -820,17 +822,49 @@ impl ServerContext { } if frame.stream_id == StreamId::CONNECTION { - debug!("TODO: ignoring connection-wide window update"); + let new_capacity = match self.state.outgoing_capacity.checked_add(increment) { + Some(x) => x, + None => { + return Err(H2ConnectionError::WindowUpdateOverflow); + } + }; + debug!(old_capacity = %self.state.outgoing_capacity, %new_capacity, "connection window update"); + self.state.outgoing_capacity = new_capacity; + self.state.send_data_maybe.notify_one(); } else { - match self.state.streams.get_mut(&frame.stream_id) { + let outgoing = match self + .state + .streams + .get_mut(&frame.stream_id) + .and_then(|ss| ss.outgoing_mut()) + { + Some(ss) => ss, None => { - return Err(H2ConnectionError::WindowUpdateForUnknownStream { + return Err(H2ConnectionError::WindowUpdateForUnknownOrClosedStream { stream_id: frame.stream_id, }); } - Some(_ss) => { - debug!("TODO: handle window update for stream {}", frame.stream_id) + }; + + let new_capacity = match outgoing.capacity.checked_add(increment) { + Some(x) => x, + None => { + // reset the stream + self.rst(frame.stream_id, H2StreamError::WindowUpdateOverflow) + .await?; + return Ok(()); } + }; + + debug!(old_capacity = %outgoing.capacity, %new_capacity, "stream window update"); + outgoing.capacity = new_capacity; + if self.state.outgoing_capacity > 0 + && self + .state + .streams_with_pending_data + .contains(&frame.stream_id) + { + self.state.send_data_maybe.notify_one(); } } } @@ -1097,7 +1131,11 @@ impl ServerContext { rx: piece_rx, }; - let incoming = piece_tx; + let incoming = StreamIncoming { + // FIXME: handle negative window size, cf. https://httpwg.org/specs/rfc9113.html#InitialWindowSize + capacity: self.state.peer_settings.initial_window_size, + tx: piece_tx, + }; let outgoing: StreamOutgoing = self.state.mk_stream_outgoing(); self.state.streams.insert( @@ -1144,6 +1182,7 @@ impl ServerContext { match self.state.streams.get_mut(&stream_id) { Some(StreamState::Open { incoming, .. }) => { if incoming + .tx .send(Ok(PieceOrTrailers::Trailers(Box::new(headers)))) .await .is_err() diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index d8e4dfdb..d3f664fd 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -239,8 +239,8 @@ pub(crate) enum H2ConnectionError { #[error("client sent a push promise frame, clients aren't allowed to do that, cf. RFC9113 section 8.4")] ClientSentPushPromise, - #[error("received window update for unknown stream {stream_id}")] - WindowUpdateForUnknownStream { stream_id: StreamId }, + #[error("received window update for unknown/closed stream {stream_id}")] + WindowUpdateForUnknownOrClosedStream { stream_id: StreamId }, #[error("other error: {0:?}")] Internal(#[from] eyre::Report), @@ -275,6 +275,9 @@ pub(crate) enum H2ConnectionError { #[error("zero increment in window update frame for stream")] WindowUpdateZeroIncrement, + #[error("received window update that made the window size overflow")] + WindowUpdateOverflow, + #[error("received window update frame with invalid length {len}")] WindowUpdateInvalidLength { len: usize }, } @@ -289,6 +292,8 @@ impl H2ConnectionError { H2ConnectionError::PingFrameInvalidLength { .. } => KnownErrorCode::FrameSizeError, H2ConnectionError::SettingsAckWithPayload { .. } => KnownErrorCode::FrameSizeError, H2ConnectionError::WindowUpdateInvalidLength { .. } => KnownErrorCode::FrameSizeError, + // flow control errors + H2ConnectionError::WindowUpdateOverflow => KnownErrorCode::FlowControlError, // compression errors H2ConnectionError::CompressionError(_) => KnownErrorCode::CompressionError, // stream closed error @@ -327,6 +332,9 @@ pub(crate) enum H2StreamError { #[error("received RST_STREAM frame with invalid size, expected 4 got {frame_size}")] InvalidRstStreamFrameSize { frame_size: u32 }, + + #[error("received WINDOW_UPDATE that made the window size overflow")] + WindowUpdateOverflow, } impl H2StreamError { @@ -335,10 +343,15 @@ impl H2StreamError { use KnownErrorCode as Code; match self { + // stream closed error StreamClosed => Code::StreamClosed, + // stream refused error RefusedStream => Code::RefusedStream, + // frame size errors InvalidPriorityFrameSize { .. } => Code::FrameSizeError, InvalidRstStreamFrameSize { .. } => Code::FrameSizeError, + // flow control errors + WindowUpdateOverflow => Code::FlowControlError, _ => Code::ProtocolError, } } From 959ffde72c711d8b01b29762b8317aa0f507072d Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Mon, 18 Mar 2024 17:26:46 +0100 Subject: [PATCH 04/25] Remove erroneous/outdated comment (the Frame struct no longer contains the payload) --- crates/fluke/src/h2/parse.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/fluke/src/h2/parse.rs b/crates/fluke/src/h2/parse.rs index 3087e632..b19d43dc 100644 --- a/crates/fluke/src/h2/parse.rs +++ b/crates/fluke/src/h2/parse.rs @@ -311,9 +311,7 @@ impl Frame { self } - /// Parse a frame from the given slice. This also takes the payload from the - /// slice, and copies it to the heap, which may not be ideal for a production - /// implementation. + /// Parse a frame from the given slice pub fn parse(i: Roll) -> IResult { let (i, (len, frame_type, (reserved, stream_id))) = tuple(( be_u24, From fdca00ee28d39af90434fa87066175b1375a0619 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Mon, 18 Mar 2024 17:30:17 +0100 Subject: [PATCH 05/25] Allow passing arguments to just curl-tests --- Justfile | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/Justfile b/Justfile index b2e5076c..01236ebb 100644 --- a/Justfile +++ b/Justfile @@ -20,12 +20,12 @@ test *args: just build-testbed RUST_BACKTRACE=1 cargo nextest run {{args}} -curl-tests: - RUST_BACKTRACE=1 cargo nextest run --manifest-path test-crates/fluke-curl-tests/Cargo.toml +curl-tests *args: + RUST_BACKTRACE=1 cargo nextest run --manifest-path test-crates/fluke-curl-tests/Cargo.toml {{args}} build-testbed: cargo build --release --manifest-path test-crates/hyper-testbed/Cargo.toml - + single-test *args: just test --no-capture {{args}} @@ -33,7 +33,7 @@ bench *args: RUST_BACKTRACE=1 cargo bench {{args}} -- --plotting-backend plotters h2spec *args: - #!/bin/bash -eux + #!/bin/bash -eux export RUST_LOG="${RUST_LOG:-fluke=debug,fluke_hpack=info}" export RUST_BACKTRACE="${RUST_BACKTRACE:-1}" cargo run --manifest-path test-crates/fluke-h2spec/Cargo.toml -- {{args}} @@ -57,4 +57,3 @@ check: ktls-sample: cargo run --manifest-path test-crates/fluke-tls-sample/Cargo.toml - From 46e81cd66db9509d37ed13fcb583bc1d7b5ced49 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Mon, 18 Mar 2024 17:30:39 +0100 Subject: [PATCH 06/25] Better colored backtrace + tracing-subscriber setup for fluke-curl-tests --- test-crates/fluke-curl-tests/Cargo.lock | 51 +++++++++++++++++++ test-crates/fluke-curl-tests/Cargo.toml | 1 + .../fluke-curl-tests/tests/helpers/mod.rs | 1 + .../tests/helpers/tracing_common.rs | 8 +-- .../tests/integration_test.rs | 10 ---- 5 files changed, 57 insertions(+), 14 deletions(-) diff --git a/test-crates/fluke-curl-tests/Cargo.lock b/test-crates/fluke-curl-tests/Cargo.lock index 89a93552..373cef43 100644 --- a/test-crates/fluke-curl-tests/Cargo.lock +++ b/test-crates/fluke-curl-tests/Cargo.lock @@ -74,6 +74,33 @@ version = "1.0.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "baf1de4339761588bc0619e3cbc0120ee582ebb74b53b4efbf79117bd2da40fd" +[[package]] +name = "color-eyre" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "55146f5e46f237f7423d74111267d4597b59b0dad0ffaf7303bce9945d843ad5" +dependencies = [ + "backtrace", + "color-spantrace", + "eyre", + "indenter", + "once_cell", + "owo-colors", + "tracing-error", +] + +[[package]] +name = "color-spantrace" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cd6be1b2a7e382e2b98b43b2adcca6bb0e465af0bdd38123873ae61eb17a72c2" +dependencies = [ + "once_cell", + "owo-colors", + "tracing-core", + "tracing-error", +] + [[package]] name = "curl" version = "0.4.46" @@ -192,6 +219,7 @@ name = "fluke-curl-tests" version = "0.1.0" dependencies = [ "bytes", + "color-eyre", "curl", "eyre", "fluke", @@ -427,6 +455,12 @@ version = "0.1.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b15813163c1d831bf4a13c3610c05c0d03b39feb07f7e09fa234dac9b15aaf39" +[[package]] +name = "owo-colors" +version = "3.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c1b04fb49957986fdce4d6ee7a65027d55d4b6d2265e5848bbb507b58ccfdb6f" + [[package]] name = "pin-project-lite" version = "0.2.13" @@ -680,6 +714,17 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "c06d3da6113f116aaee68e4d601191614c9053067f9ab7f6edbcb161237daa54" dependencies = [ "once_cell", + "valuable", +] + +[[package]] +name = "tracing-error" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d686ec1c0f384b1277f097b2f279a2ecc11afe8c133c1aabf036a27cb4cd206e" +dependencies = [ + "tracing", + "tracing-subscriber", ] [[package]] @@ -700,6 +745,12 @@ version = "1.0.12" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3354b9ac3fae1ff6755cb6db53683adb661634f67557942dea4facebec0fee4b" +[[package]] +name = "valuable" +version = "0.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "830b7e5d4d90034032940e4ace0d9a9a057e7a45cd94e6c007832e39edb82f6d" + [[package]] name = "vcpkg" version = "0.2.15" diff --git a/test-crates/fluke-curl-tests/Cargo.toml b/test-crates/fluke-curl-tests/Cargo.toml index 7b2dd00b..01fdb5f5 100644 --- a/test-crates/fluke-curl-tests/Cargo.toml +++ b/test-crates/fluke-curl-tests/Cargo.toml @@ -22,3 +22,4 @@ eyre = { version = "0.6.12", default-features = false } tracing = "0.1.40" http = "1.1.0" pretty-hex = "0.4.1" +color-eyre = "0.6.3" diff --git a/test-crates/fluke-curl-tests/tests/helpers/mod.rs b/test-crates/fluke-curl-tests/tests/helpers/mod.rs index ac67207d..5848ce74 100644 --- a/test-crates/fluke-curl-tests/tests/helpers/mod.rs +++ b/test-crates/fluke-curl-tests/tests/helpers/mod.rs @@ -3,6 +3,7 @@ use std::future::Future; pub(crate) mod tracing_common; pub(crate) fn run(test: impl Future>) { + color_eyre::install().unwrap(); fluke::maybe_uring::start(async { tracing_common::setup_tracing(); diff --git a/test-crates/fluke-curl-tests/tests/helpers/tracing_common.rs b/test-crates/fluke-curl-tests/tests/helpers/tracing_common.rs index a7ad7fb6..208b6336 100644 --- a/test-crates/fluke-curl-tests/tests/helpers/tracing_common.rs +++ b/test-crates/fluke-curl-tests/tests/helpers/tracing_common.rs @@ -7,14 +7,14 @@ use tracing_subscriber::{filter::Targets, layer::SubscriberExt, util::Subscriber /// test, which is a limitation we accept. pub(crate) fn setup_tracing() { let filter_layer = Targets::new() - .with_default(Level::TRACE) - // .with_target("fluke", Level::TRACE) + .with_default(Level::INFO) .with_target("fluke", Level::DEBUG) .with_target("want", Level::INFO); let fmt_layer = tracing_subscriber::fmt::layer() - .with_file(true) - .with_line_number(true); + .pretty() + .with_file(false) + .with_line_number(false); tracing_subscriber::registry() .with(filter_layer) diff --git a/test-crates/fluke-curl-tests/tests/integration_test.rs b/test-crates/fluke-curl-tests/tests/integration_test.rs index 1ecba277..fe894fe2 100644 --- a/test-crates/fluke-curl-tests/tests/integration_test.rs +++ b/test-crates/fluke-curl-tests/tests/integration_test.rs @@ -370,8 +370,6 @@ fn proxy_echo_body_content_len() { #[test] fn proxy_echo_body_chunked() { - eyre::set_hook(Box::new(|e| eyre::DefaultHandler::default_with(e))).unwrap(); - #[allow(drop_bounds)] async fn client(ln_addr: SocketAddr, _guard: impl Drop) -> eyre::Result<()> { let socket = TcpStream::connect(ln_addr).await?; @@ -531,8 +529,6 @@ fn curl_echo_body_chunked() { } fn curl_echo_body(typ: BodyType) { - eyre::set_hook(Box::new(|e| eyre::DefaultHandler::default_with(e))).unwrap(); - #[allow(drop_bounds)] fn client(typ: BodyType, ln_addr: SocketAddr, _guard: impl Drop) -> eyre::Result<()> { let req_body = "Please return to sender".as_bytes(); @@ -615,8 +611,6 @@ fn curl_echo_body_noproxy_chunked() { } fn curl_echo_body_noproxy(typ: BodyType) { - eyre::set_hook(Box::new(|e| eyre::DefaultHandler::default_with(e))).unwrap(); - #[allow(drop_bounds)] fn client(typ: BodyType, ln_addr: SocketAddr, _guard: impl Drop) -> eyre::Result<()> { let req_body = "Please return to sender".as_bytes(); @@ -788,8 +782,6 @@ fn curl_echo_body_noproxy(typ: BodyType) { #[test] fn h2_basic_post() { - eyre::set_hook(Box::new(|e| eyre::DefaultHandler::default_with(e))).unwrap(); - #[allow(drop_bounds)] fn client(ln_addr: SocketAddr, _guard: impl Drop) -> eyre::Result<()> { let req_body = "Please return to sender".as_bytes(); @@ -995,8 +987,6 @@ impl Body for SampleBody { // TODO: dedup with h2_basic_post #[test] fn h2_basic_get() { - eyre::set_hook(Box::new(|e| eyre::DefaultHandler::default_with(e))).unwrap(); - #[allow(drop_bounds)] fn client(ln_addr: SocketAddr, _guard: impl Drop) -> eyre::Result<()> { let mut res_body = Vec::new(); From 31ec2a19b5e7e42076f152fcc9fa82cd9fec1e7b Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Mon, 18 Mar 2024 17:41:49 +0100 Subject: [PATCH 07/25] BodyEnd sets eof to true --- crates/fluke/src/h2/server.rs | 55 ++++++++++++++++++++++------------- crates/fluke/src/h2/types.rs | 10 +++++++ 2 files changed, 44 insertions(+), 21 deletions(-) diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index b505d048..adaf2948 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -395,7 +395,12 @@ impl ServerContext { self.write_frame(frame, payload).await?; } H2EventPayload::BodyChunk(chunk) => { - let stream = match self.state.streams.get_mut(&ev.stream_id) { + let outgoing = match self + .state + .streams + .get_mut(&ev.stream_id) + .and_then(|s| s.outgoing_mut()) + { None => { // ignore the event then, but at this point we should // tell the sender to stop sending chunks, which is not @@ -403,32 +408,40 @@ impl ServerContext { // TODO: make it possible to propagate errors to the sender return Ok(()); } - Some(stream) => stream, + Some(outgoing) => outgoing, }; - if let Some(outgoing) = stream.outgoing_mut() { - outgoing.pieces.push(chunk); - self.state.streams_with_pending_data.insert(ev.stream_id); - if self.state.outgoing_capacity > 0 && outgoing.capacity > 0 { - // worth revisiting then! - self.state.send_data_maybe.notify_one(); - } + if outgoing.eof { + unreachable!("got a body chunk after the body end event!") } - // let flags = BitFlags::::default(); - // let frame = Frame::new(FrameType::Data(flags), ev.stream_id); - - // self.write_frame(frame, chunk).await?; + outgoing.pieces.push(chunk); + self.state.streams_with_pending_data.insert(ev.stream_id); + if self.state.outgoing_capacity > 0 && outgoing.capacity > 0 { + // worth revisiting then! + self.state.send_data_maybe.notify_one(); + } } H2EventPayload::BodyEnd => { - // FIXME: this should transition the stream to `Closed` - // state (or at the very least `HalfClosedLocal`). - // Either way, whoever owns the stream state should know - // about it, cf. https://github.com/bearcove/fluke/issues/123 - - let flags = DataFlags::EndStream; - let frame = Frame::new(FrameType::Data(flags.into()), ev.stream_id); - self.write_frame(frame, Roll::empty()).await?; + let outgoing = match self + .state + .streams + .get_mut(&ev.stream_id) + .and_then(|s| s.outgoing_mut()) + { + None => return Ok(()), + Some(outgoing) => outgoing, + }; + + if outgoing.eof { + panic!("got a body end event after the body end event!") + } + + outgoing.eof = true; + if outgoing.is_empty() { + // we'll need to send a zero-length data frame + self.state.send_data_maybe.notify_one(); + } } } diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index d3f664fd..e6041743 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -178,6 +178,16 @@ pub(crate) struct StreamOutgoing { // window size of the stream, ie. how many bytes // we can send to the receiver before waiting. pub(crate) capacity: u32, + + // true if the stream has been closed (ie. all the pieces + // have been sent and the receiver has been notified) + pub(crate) eof: bool, +} + +impl StreamOutgoing { + pub(crate) fn is_empty(&self) -> bool { + self.pieces.is_empty() + } } #[derive(Debug, thiserror::Error)] From cf38f9efb0a382abb81a1afed97909341f79afa1 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Mon, 18 Mar 2024 17:44:15 +0100 Subject: [PATCH 08/25] a little forgetfulness --- Justfile | 2 +- crates/fluke/src/h2/types.rs | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/Justfile b/Justfile index 01236ebb..54d6ddd3 100644 --- a/Justfile +++ b/Justfile @@ -21,7 +21,7 @@ test *args: RUST_BACKTRACE=1 cargo nextest run {{args}} curl-tests *args: - RUST_BACKTRACE=1 cargo nextest run --manifest-path test-crates/fluke-curl-tests/Cargo.toml {{args}} + RUST_BACKTRACE=1 cargo nextest run --no-capture --manifest-path test-crates/fluke-curl-tests/Cargo.toml {{args}} build-testbed: cargo build --release --manifest-path test-crates/hyper-testbed/Cargo.toml diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index e6041743..3ff2747f 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -60,6 +60,7 @@ impl ConnState { pieces: Vec::new(), offset: 0, capacity: self.self_settings.initial_window_size, + eof: false, } } } From 49a571b2e431aaf44e029f3cd54c0f7be67ddcf0 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Mon, 18 Mar 2024 18:22:02 +0100 Subject: [PATCH 09/25] Ready to write --- crates/fluke/src/h2/parse.rs | 1 - crates/fluke/src/h2/server.rs | 120 +++++++++++++++--- crates/fluke/src/h2/types.rs | 2 +- .../tests/helpers/tracing_common.rs | 2 +- 4 files changed, 107 insertions(+), 18 deletions(-) diff --git a/crates/fluke/src/h2/parse.rs b/crates/fluke/src/h2/parse.rs index b19d43dc..76b1dcfd 100644 --- a/crates/fluke/src/h2/parse.rs +++ b/crates/fluke/src/h2/parse.rs @@ -605,7 +605,6 @@ impl Settings { } SettingIdentifier::MaxConcurrentStreams => { settings.max_concurrent_streams = value; - tracing::warn!("parsed max concurrent streams {value}"); } SettingIdentifier::InitialWindowSize => { if value > Self::MAX_INITIAL_WINDOW_SIZE { diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index adaf2948..b1aa7f4d 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -1,5 +1,6 @@ use std::{ borrow::Cow, + collections::HashSet, io::Write, net::Shutdown, rc::Rc, @@ -86,6 +87,28 @@ pub(crate) struct ServerContext { ev_rx: mpsc::Receiver, } +impl ConnState { + fn eof_streams(&self) -> HashSet { + self.streams_with_pending_data + .iter() + .copied() + .filter_map(|id| { + self.streams + .get(&id) + .and_then(|ss| ss.outgoing_ref()) + .map(|og| (id, og)) + }) + .filter_map(|(id, outgoing)| { + if outgoing.eof && outgoing.is_empty() { + Some(id) + } else { + None + } + }) + .collect() + } +} + impl ServerContext { pub(crate) fn new(driver: Rc, state: ConnState, transport_w: W) -> eyre::Result { let mut hpack_dec = fluke_hpack::Decoder::new(); @@ -353,6 +376,10 @@ impl ServerContext { } } + _ = self.state.send_data_maybe.notified() => { + self.send_data_maybe().await?; + } + ev = self.ev_rx.recv() => { match ev { Some(ev) => self.handle_event(ev).await?, @@ -365,6 +392,61 @@ impl ServerContext { Ok(()) } + async fn send_data_maybe(&mut self) -> Result<(), H2ConnectionError> { + let eof_streams: HashSet = self.state.eof_streams(); + for id in eof_streams { + let flags = DataFlags::EndStream; + let frame = Frame::new(FrameType::Data(flags.into()), id); + self.write_frame(frame, Roll::empty()).await?; + } + + if self.state.outgoing_capacity == 0 { + // that's all we can do + return Ok(()); + } + + let mut not_pending: HashSet = Default::default(); + + for id in self.state.streams_with_pending_data.iter().copied() { + let outgoing = self + .state + .streams + .get_mut(&id) + .and_then(|ss| ss.outgoing_mut()) + .expect("stream should not be in streams_with_pending_data if it's already closed / not in an outgoing state"); + + if outgoing.is_empty() { + not_pending.insert(id); + continue; + } + + // how much data do we have available? + let outg_len: u32 = outgoing + .pieces + .iter() + .map(|p| p.len()) + .sum::() + .try_into() + .unwrap_or(u32::MAX); + + // how much data are we allowed to write? + let conn_cap = self.state.outgoing_capacity; + let strm_cap = outgoing.capacity; + let max_fram = self.state.peer_settings.max_frame_size; + + let write_size = conn_cap.min(strm_cap).min(max_fram).min(outg_len); + + debug!(%outg_len, %conn_cap, %strm_cap, %max_fram, %write_size, "ready to write"); + todo!(); + } + + for id in not_pending { + self.state.streams_with_pending_data.remove(&id); + } + + Ok(()) + } + async fn handle_event(&mut self, ev: H2Event) -> Result<(), H2ConnectionError> { match ev.payload { H2EventPayload::Headers(res) => { @@ -457,29 +539,37 @@ impl ServerContext { let payload = payload.into(); match &frame.frame_type { - FrameType::Data(headers) => { - if headers.contains(DataFlags::EndStream) { - // if the stream is open, this transitions to HalfClosedLocal. - if let Some(ss) = self.state.streams.get_mut(&frame.stream_id) { - match ss { + FrameType::Data(flags) => { + if flags.contains(DataFlags::EndStream) { + // we won't be sending any more data on this stream + self.state + .streams_with_pending_data + .remove(&frame.stream_id); + + if let std::collections::hash_map::Entry::Occupied(mut ss) = + self.state.streams.entry(frame.stream_id) + { + match ss.get_mut() { StreamState::Open { .. } => { - let incoming = match std::mem::take(ss) { + let incoming = match std::mem::take(ss.get_mut()) { StreamState::Open { incoming, .. } => incoming, _ => unreachable!(), }; - *ss = StreamState::HalfClosedLocal { incoming }; + // this avoid having to re-insert the stream in the map + *ss.get_mut() = StreamState::HalfClosedLocal { incoming }; } _ => { // transition to closed - if self.state.streams.remove(&frame.stream_id).is_some() { - debug!( - "Closed stream {} (wrote data w/EndStream), now have {} streams", - frame.stream_id, - self.state.streams.len() - ); - } + ss.remove(); + debug!( + "Closed stream {} (wrote data w/EndStream), now have {} streams", + frame.stream_id, + self.state.streams.len() + ); } } + } else { + unreachable!("stream was in streams_with_pending_data but not in streams") } } } @@ -1146,7 +1236,7 @@ impl ServerContext { let incoming = StreamIncoming { // FIXME: handle negative window size, cf. https://httpwg.org/specs/rfc9113.html#InitialWindowSize - capacity: self.state.peer_settings.initial_window_size, + capacity: self.state.self_settings.initial_window_size, tx: piece_tx, }; let outgoing: StreamOutgoing = self.state.mk_stream_outgoing(); diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index 3ff2747f..8be81c5a 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -59,7 +59,7 @@ impl ConnState { StreamOutgoing { pieces: Vec::new(), offset: 0, - capacity: self.self_settings.initial_window_size, + capacity: self.peer_settings.initial_window_size, eof: false, } } diff --git a/test-crates/fluke-curl-tests/tests/helpers/tracing_common.rs b/test-crates/fluke-curl-tests/tests/helpers/tracing_common.rs index 208b6336..b1c7f7f4 100644 --- a/test-crates/fluke-curl-tests/tests/helpers/tracing_common.rs +++ b/test-crates/fluke-curl-tests/tests/helpers/tracing_common.rs @@ -12,7 +12,7 @@ pub(crate) fn setup_tracing() { .with_target("want", Level::INFO); let fmt_layer = tracing_subscriber::fmt::layer() - .pretty() + .with_ansi(true) .with_file(false) .with_line_number(false); From e63aef82bef6eaf5c8ff8d98350dbfd5f1f9c124 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Mon, 18 Mar 2024 18:47:44 +0100 Subject: [PATCH 10/25] drat, partial writes. --- crates/fluke-buffet/src/piece.rs | 1 + crates/fluke/src/h2/server.rs | 125 +++++++++++++++++++++++-------- crates/fluke/src/h2/types.rs | 10 +-- 3 files changed, 97 insertions(+), 39 deletions(-) diff --git a/crates/fluke-buffet/src/piece.rs b/crates/fluke-buffet/src/piece.rs index e69deb50..f6e2ff9e 100644 --- a/crates/fluke-buffet/src/piece.rs +++ b/crates/fluke-buffet/src/piece.rs @@ -138,6 +138,7 @@ impl Piece { /// A list of [Piece], suitable for issuing vectored writes via io_uring. #[derive(Default)] pub struct PieceList { + // TODO: use smallvec? pieces: Vec, } diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index b1aa7f4d..60a50b5b 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -376,16 +376,16 @@ impl ServerContext { } } - _ = self.state.send_data_maybe.notified() => { - self.send_data_maybe().await?; - } - ev = self.ev_rx.recv() => { match ev { Some(ev) => self.handle_event(ev).await?, None => unreachable!("the context owns a copy of the sender, and this method has &mut self, so the sender can't be dropped while this method is running"), } - }, + } + + _ = self.state.send_data_maybe.notified() => { + self.send_data_maybe().await?; + } } } @@ -407,7 +407,14 @@ impl ServerContext { let mut not_pending: HashSet = Default::default(); - for id in self.state.streams_with_pending_data.iter().copied() { + let streams_with_pending_data: HashSet<_> = self + .state + .streams_with_pending_data + .iter() + .copied() + .collect(); + + for id in streams_with_pending_data { let outgoing = self .state .streams @@ -437,7 +444,27 @@ impl ServerContext { let write_size = conn_cap.min(strm_cap).min(max_fram).min(outg_len); debug!(%outg_len, %conn_cap, %strm_cap, %max_fram, %write_size, "ready to write"); - todo!(); + if outg_len == write_size { + // yay we can just send everything + // TODO: send a PieceList and do a single write instead + // TODO: actually respect max frame size + + let mut pieces = std::mem::take(&mut outgoing.pieces); + let eof = outgoing.eof; + let num_pieces = pieces.len(); + + for (i, piece) in pieces.drain(..).enumerate() { + let flags: BitFlags = if eof && i == num_pieces - 1 { + DataFlags::EndStream.into() + } else { + Default::default() + }; + let frame = Frame::new(FrameType::Data(flags.into()), id); + self.write_frame(frame, piece).await?; + } + } else { + todo!("handle partial writes") + } } for id in not_pending { @@ -497,7 +524,11 @@ impl ServerContext { unreachable!("got a body chunk after the body end event!") } - outgoing.pieces.push(chunk); + // FIXME: this isn't great, because, due to biased polling, body pieces can pile up. + // when we've collected enough pieces for max frame size, we should really send them. + outgoing.pieces.push_back(chunk); + debug!(stream_id = %ev.stream_id, pending_chunks = %outgoing.pieces.len(), "got a body chunk"); + self.state.streams_with_pending_data.insert(ev.stream_id); if self.state.outgoing_capacity > 0 && outgoing.capacity > 0 { // worth revisiting then! @@ -519,6 +550,7 @@ impl ServerContext { panic!("got a body end event after the body end event!") } + debug!(stream_id = %ev.stream_id, pending_chunks = %outgoing.pieces.len(), "got body end"); outgoing.eof = true; if outgoing.is_empty() { // we'll need to send a zero-length data frame @@ -535,41 +567,69 @@ impl ServerContext { mut frame: Frame, payload: impl Into, ) -> Result<(), H2ConnectionError> { - debug!(?frame, ">"); let payload = payload.into(); match &frame.frame_type { FrameType::Data(flags) => { + let mut ss = match self.state.streams.entry(frame.stream_id) { + std::collections::hash_map::Entry::Occupied(entry) => entry, + std::collections::hash_map::Entry::Vacant(_) => unreachable!( + "writing DATA frame for non-existent stream, this should never happen" + ), + }; + + // update stream flow control window + { + let outgoing = match ss.get_mut().outgoing_mut() { + Some(og) => og, + None => unreachable!("writing DATA frame for stream in the wrong state"), + }; + let payload_len: u32 = payload.len().try_into().unwrap(); + if let Some(next_cap) = outgoing.capacity.checked_sub(payload_len) { + outgoing.capacity = next_cap; + } else { + unreachable!( + "should never write a frame that makes the stream capacity negative" + ) + } + } + + // now update connection flow control window + { + let payload_len: u32 = payload.len().try_into().unwrap(); + if let Some(next_cap) = self.state.outgoing_capacity.checked_sub(payload_len) { + self.state.outgoing_capacity = next_cap; + } else { + unreachable!( + "should never write a frame that makes the connection capacity negative" + ) + } + } + if flags.contains(DataFlags::EndStream) { // we won't be sending any more data on this stream self.state .streams_with_pending_data .remove(&frame.stream_id); - if let std::collections::hash_map::Entry::Occupied(mut ss) = - self.state.streams.entry(frame.stream_id) - { - match ss.get_mut() { - StreamState::Open { .. } => { - let incoming = match std::mem::take(ss.get_mut()) { - StreamState::Open { incoming, .. } => incoming, - _ => unreachable!(), - }; - // this avoid having to re-insert the stream in the map - *ss.get_mut() = StreamState::HalfClosedLocal { incoming }; - } - _ => { - // transition to closed - ss.remove(); - debug!( - "Closed stream {} (wrote data w/EndStream), now have {} streams", - frame.stream_id, - self.state.streams.len() - ); - } + match ss.get_mut() { + StreamState::Open { .. } => { + let incoming = match std::mem::take(ss.get_mut()) { + StreamState::Open { incoming, .. } => incoming, + _ => unreachable!(), + }; + // this avoid having to re-insert the stream in the map + *ss.get_mut() = StreamState::HalfClosedLocal { incoming }; + } + _ => { + // transition to closed + ss.remove(); + debug!( + "Closed stream {} (wrote data w/EndStream), now have {} streams", + frame.stream_id, + self.state.streams.len() + ); } - } else { - unreachable!("stream was in streams_with_pending_data but not in streams") } } } @@ -590,6 +650,7 @@ impl ServerContext { frame_size: payload.len() as _, max_frame_size: u32::MAX, })?; + debug!(?frame, ">"); let frame_roll = frame.into_roll(&mut self.out_scratch)?; if payload.is_empty() { diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index 8be81c5a..949fbe93 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -1,5 +1,5 @@ use std::{ - collections::{HashMap, HashSet}, + collections::{HashMap, HashSet, VecDeque}, fmt, }; @@ -57,8 +57,7 @@ impl ConnState { /// create a new [StreamOutgoing] based on our current settings pub(crate) fn mk_stream_outgoing(&self) -> StreamOutgoing { StreamOutgoing { - pieces: Vec::new(), - offset: 0, + pieces: Default::default(), capacity: self.peer_settings.initial_window_size, eof: false, } @@ -171,10 +170,7 @@ impl StreamState { pub(crate) struct StreamOutgoing { // list of pieces we need to send out - pub(crate) pieces: Vec, - - // offset within the first piece - pub(crate) offset: usize, + pub(crate) pieces: VecDeque, // window size of the stream, ie. how many bytes // we can send to the receiver before waiting. From ccac5c07b478bed250aa3ebe105e062aadd9dffb Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 12:57:21 +0100 Subject: [PATCH 11/25] Add cargo-nextest to nix flake --- flake.nix | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/flake.nix b/flake.nix index bcd48e57..e8f86e24 100644 --- a/flake.nix +++ b/flake.nix @@ -68,7 +68,7 @@ }; devShells.default = mkShell { inputsFrom = [ bin ]; - packages = with pkgs; [ just nixpkgs-fmt ]; + packages = with pkgs; [ just nixpkgs-fmt cargo-nextest ]; }; } ); From 1adea9d9466b9c4999533da44cc9eeaa28ae15f7 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 13:51:08 +0100 Subject: [PATCH 12/25] wip: Piece::Slice --- Justfile | 1 + crates/fluke-buffet/src/piece.rs | 225 +++++++++++++++++++++++++------ crates/fluke/src/h2/server.rs | 2 +- crates/fluke/src/types/mod.rs | 4 +- 4 files changed, 189 insertions(+), 43 deletions(-) diff --git a/Justfile b/Justfile index 54d6ddd3..471ebf24 100644 --- a/Justfile +++ b/Justfile @@ -21,6 +21,7 @@ test *args: RUST_BACKTRACE=1 cargo nextest run {{args}} curl-tests *args: + just build-testbed RUST_BACKTRACE=1 cargo nextest run --no-capture --manifest-path test-crates/fluke-curl-tests/Cargo.toml {{args}} build-testbed: diff --git a/crates/fluke-buffet/src/piece.rs b/crates/fluke-buffet/src/piece.rs index f6e2ff9e..976fc1d7 100644 --- a/crates/fluke-buffet/src/piece.rs +++ b/crates/fluke-buffet/src/piece.rs @@ -1,6 +1,6 @@ //! Types for performing vectored I/O. -use std::{fmt, ops::Deref, str::Utf8Error}; +use std::{fmt, ops::Deref, rc::Rc, str::Utf8Error}; use fluke_maybe_uring::buf::IoBuf; use http::header::HeaderName; @@ -11,33 +11,54 @@ use crate::{Roll, RollStr}; /// passing to the kernel (io_uring writes). #[derive(Clone)] pub enum Piece { + Full { + core: PieceCore, + }, + Slice { + core: PieceCore, + start: usize, + len: usize, + }, +} + +#[derive(Clone)] +pub enum PieceCore { Static(&'static [u8]), - Vec(Vec), + Vec(Rc>), Roll(Roll), HeaderName(HeaderName), } -impl From<&'static [u8]> for Piece { +impl From for Piece +where + T: Into, +{ + fn from(t: T) -> Self { + Piece::Full { core: t.into() } + } +} + +impl From<&'static [u8]> for PieceCore { fn from(slice: &'static [u8]) -> Self { - Piece::Static(slice) + PieceCore::Static(slice) } } -impl From<&'static str> for Piece { +impl From<&'static str> for PieceCore { fn from(slice: &'static str) -> Self { - Piece::Static(slice.as_bytes()) + PieceCore::Static(slice.as_bytes()) } } -impl From> for Piece { +impl From> for PieceCore { fn from(vec: Vec) -> Self { - Piece::Vec(vec) + PieceCore::Vec(Rc::new(vec)) } } -impl From for Piece { +impl From for PieceCore { fn from(roll: Roll) -> Self { - Piece::Roll(roll) + PieceCore::Roll(roll) } } @@ -47,40 +68,105 @@ impl From for Piece { } } -impl From for Piece { +impl From for PieceCore { fn from(name: HeaderName) -> Self { - Piece::HeaderName(name) + PieceCore::HeaderName(name) } } impl Deref for Piece { type Target = [u8]; + fn deref(&self) -> &Self::Target { + self.core().deref() + } +} + +impl Deref for PieceCore { + type Target = [u8]; + fn deref(&self) -> &Self::Target { self.as_ref() } } -impl AsRef<[u8]> for Piece { +impl AsRef<[u8]> for PieceCore { fn as_ref(&self) -> &[u8] { match self { - Piece::Static(slice) => slice, - Piece::Vec(vec) => vec.as_ref(), - Piece::Roll(roll) => roll.as_ref(), - Piece::HeaderName(name) => name.as_str().as_bytes(), + PieceCore::Static(slice) => slice, + PieceCore::Vec(vec) => vec.as_ref(), + PieceCore::Roll(roll) => roll.as_ref(), + PieceCore::HeaderName(name) => name.as_str().as_bytes(), } } } impl Piece { - /// Decode as utf-8 (borrowed) - pub fn as_str(&self) -> Result<&str, Utf8Error> { - std::str::from_utf8(self.as_ref()) + fn start(&self) -> usize { + match self { + Piece::Full { .. } => 0, + Piece::Slice { start, .. } => *start, + } + } + + fn core(&self) -> &PieceCore { + match self { + Piece::Full { core } => core, + Piece::Slice { core, .. } => core, + } + } + + /// Split the piece into two at the given index. + /// The original piece will be consumed. + /// Returns a tuple of the two pieces. + pub fn split_at(self, middle: usize) -> (Self, Self) { + let len = self.len(); + assert!(middle <= len); + + match self { + Piece::Full { core } => ( + Self::Slice { + core: core.clone(), + start: 0, + len: middle, + }, + Self::Slice { + core, + start: middle, + len: len - middle, + }, + ), + Piece::Slice { core, start, len } => ( + Self::Slice { + core: core.clone(), + start, + len: middle, + }, + Self::Slice { + core, + start: start + middle, + len: len - middle, + }, + ), + } + } +} + +impl AsRef<[u8]> for Piece { + fn as_ref(&self) -> &[u8] { + let ptr = self.core().as_ref(); + if let Piece::Slice { start, len, .. } = self { + &ptr[*start..][..*len] + } else { + ptr + } } +} - /// Decode as utf-8 (owned) +impl Piece { + // Decode as utf-8 (owned) pub fn to_str(self) -> Result { - _ = std::str::from_utf8(&self)?; + _ = std::str::from_utf8(&self[..])?; Ok(PieceStr { piece: self }) } @@ -93,34 +179,55 @@ impl Piece { } } -unsafe impl IoBuf for Piece { +unsafe impl IoBuf for PieceCore { #[inline(always)] fn stable_ptr(&self) -> *const u8 { match self { - Piece::Static(s) => IoBuf::stable_ptr(s), - Piece::Vec(s) => IoBuf::stable_ptr(s), - Piece::Roll(s) => IoBuf::stable_ptr(s), - Piece::HeaderName(s) => s.as_str().as_ptr(), + PieceCore::Static(s) => IoBuf::stable_ptr(s), + PieceCore::Vec(s) => IoBuf::stable_ptr(s.as_ref()), + PieceCore::Roll(s) => IoBuf::stable_ptr(s), + PieceCore::HeaderName(s) => s.as_str().as_ptr(), } } fn bytes_init(&self) -> usize { match self { - Piece::Static(s) => IoBuf::bytes_init(s), - Piece::Vec(s) => IoBuf::bytes_init(s), - Piece::Roll(s) => IoBuf::bytes_init(s), - Piece::HeaderName(s) => s.as_str().len(), + PieceCore::Static(s) => IoBuf::bytes_init(s), + PieceCore::Vec(s) => IoBuf::bytes_init(s.as_ref()), + PieceCore::Roll(s) => IoBuf::bytes_init(s), + PieceCore::HeaderName(s) => s.as_str().len(), } } fn bytes_total(&self) -> usize { match self { - Piece::Static(s) => IoBuf::bytes_total(s), - Piece::Vec(s) => IoBuf::bytes_total(s), - Piece::Roll(s) => IoBuf::bytes_total(s), - Piece::HeaderName(s) => s.as_str().len(), + PieceCore::Static(s) => IoBuf::bytes_total(s), + PieceCore::Vec(s) => IoBuf::bytes_total(s.as_ref()), + PieceCore::Roll(s) => IoBuf::bytes_total(s), + PieceCore::HeaderName(s) => s.as_str().len(), + } + } +} + +unsafe impl IoBuf for Piece { + #[inline(always)] + fn stable_ptr(&self) -> *const u8 { + unsafe { self.core().stable_ptr().byte_add(self.start()) } + } + + #[inline(always)] + fn bytes_init(&self) -> usize { + match self { + Piece::Full { core } => core.bytes_init(), + // TODO: triple-check + Piece::Slice { len, .. } => *len, } } + + #[inline(always)] + fn bytes_total(&self) -> usize { + todo!() + } } impl Piece { @@ -138,7 +245,11 @@ impl Piece { /// A list of [Piece], suitable for issuing vectored writes via io_uring. #[derive(Default)] pub struct PieceList { - // TODO: use smallvec? + // note: we can't use smallvec here, because the address of + // the piece list must be stable for the kernel to take + // ownership of it. + // + // we could however do our own memory pooling. pieces: Vec, } @@ -211,7 +322,7 @@ impl Deref for PieceStr { type Target = str; fn deref(&self) -> &Self::Target { - unsafe { std::str::from_utf8_unchecked(&self.piece) } + unsafe { std::str::from_utf8_unchecked(&self.piece.as_ref()) } } } @@ -236,7 +347,7 @@ impl PieceStr { impl From<&'static str> for PieceStr { fn from(s: &'static str) -> Self { PieceStr { - piece: Piece::Static(s.as_bytes()), + piece: PieceCore::Static(s.as_bytes()).into(), } } } @@ -244,7 +355,7 @@ impl From<&'static str> for PieceStr { impl From for PieceStr { fn from(s: String) -> Self { PieceStr { - piece: Piece::Vec(s.into_bytes()), + piece: PieceCore::Vec(Rc::new(s.into_bytes())).into(), } } } @@ -252,7 +363,41 @@ impl From for PieceStr { impl From for PieceStr { fn from(s: RollStr) -> Self { PieceStr { - piece: Piece::Roll(s.into_inner()), + piece: PieceCore::Roll(s.into_inner()).into(), } } } + +#[cfg(test)] +mod tests { + use crate::{Piece, PieceCore}; + + #[test] + fn test_slice() { + // test that slicing works correctly for a + // piece made from a &'static u8 + let piece: Piece = PieceCore::Static("französisch".as_bytes()).into(); + // split so that "l" is "franz" + let (first_name, last_name) = piece.split_at(5); + assert_eq!(&first_name[..], "franz".as_bytes()); + assert_eq!(&last_name[..], "ösisch".as_bytes()); + + // test edge cases, zero-length left + let piece: Piece = PieceCore::Static("französisch".as_bytes()).into(); + let (first_name, last_name) = piece.split_at(0); + assert_eq!(&first_name[..], "".as_bytes()); + assert_eq!(&last_name[..], "französisch".as_bytes()); + + // test edge cases, zero-length right + let piece: Piece = PieceCore::Static("französisch".as_bytes()).into(); + let (first_name, last_name) = piece.split_at(12); + assert_eq!(&first_name[..], "französisch".as_bytes()); + assert_eq!(&last_name[..], "".as_bytes()); + + // edge case: empty piece being split into two + let piece: Piece = PieceCore::Static(b"").into(); + let (first_name, last_name) = piece.split_at(0); + assert_eq!(&first_name[..], "".as_bytes()); + assert_eq!(&last_name[..], "".as_bytes()); + } +} diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index 60a50b5b..2a2f4249 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -1256,7 +1256,7 @@ impl ServerContext { Some(authority) => Some(authority), None => headers .get(header::HOST) - .map(|host| host.as_str().unwrap().parse().unwrap()), + .map(|host| std::str::from_utf8(host).unwrap().parse().unwrap()), }; let mut uri_parts: http::uri::Parts = Default::default(); diff --git a/crates/fluke/src/types/mod.rs b/crates/fluke/src/types/mod.rs index 7c51770a..a834735a 100644 --- a/crates/fluke/src/types/mod.rs +++ b/crates/fluke/src/types/mod.rs @@ -48,7 +48,7 @@ impl fmt::Debug for Request { .finish()?; for (name, value) in &self.headers { - debug!(%name, value = ?value.as_str(), "header"); + debug!(%name, value = ?std::str::from_utf8(value), "header"); } Ok(()) @@ -82,7 +82,7 @@ impl Response { pub(crate) fn debug_print(&self) { debug!(code = %self.status, version = ?self.version, "got response"); for (name, value) in &self.headers { - debug!(%name, value = ?value.as_str(), "got header"); + debug!(%name, value = ?std::str::from_utf8(value), "got header"); } } From 51a18931074b667fd8046dc779537b25ea732f8b Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 14:04:29 +0100 Subject: [PATCH 13/25] Finish propagating Piece/PieceCore split changes --- crates/fluke-buffet/Cargo.toml | 4 +++- crates/fluke-buffet/src/piece.rs | 6 +++--- crates/fluke/src/h1/body.rs | 4 ++-- crates/fluke/src/h1/encode.rs | 8 ++++++-- crates/fluke/src/h2/body.rs | 4 ++-- crates/fluke/src/h2/encode.rs | 2 +- crates/fluke/src/h2/server.rs | 2 +- crates/fluke/src/h2/types.rs | 4 ++-- crates/fluke/src/responder.rs | 7 ++++--- crates/fluke/src/types/mod.rs | 4 ++-- test-crates/fluke-curl-tests/tests/integration_test.rs | 6 ++++-- 11 files changed, 30 insertions(+), 21 deletions(-) diff --git a/crates/fluke-buffet/Cargo.toml b/crates/fluke-buffet/Cargo.toml index 634f8faf..2df33a3f 100644 --- a/crates/fluke-buffet/Cargo.toml +++ b/crates/fluke-buffet/Cargo.toml @@ -20,7 +20,9 @@ miri = [] [dependencies] eyre = "0.6.12" http = "1.1.0" -fluke-maybe-uring = { version = "0.1.1", path = "../fluke-maybe-uring" } +fluke-maybe-uring = { version = "0.1.1", path = "../fluke-maybe-uring", features = [ + "net", +] } memchr = "2.7.1" memmap2 = { version = "0.9.4", default-features = false } nom = "7.1.3" diff --git a/crates/fluke-buffet/src/piece.rs b/crates/fluke-buffet/src/piece.rs index 976fc1d7..63bfc3ec 100644 --- a/crates/fluke-buffet/src/piece.rs +++ b/crates/fluke-buffet/src/piece.rs @@ -74,15 +74,15 @@ impl From for PieceCore { } } -impl Deref for Piece { +impl Deref for PieceCore { type Target = [u8]; fn deref(&self) -> &Self::Target { - self.core().deref() + self.as_ref() } } -impl Deref for PieceCore { +impl Deref for Piece { type Target = [u8]; fn deref(&self) -> &Self::Target { diff --git a/crates/fluke/src/h1/body.rs b/crates/fluke/src/h1/body.rs index a61a0234..fe76b7b1 100644 --- a/crates/fluke/src/h1/body.rs +++ b/crates/fluke/src/h1/body.rs @@ -3,7 +3,7 @@ use std::fmt; use tracing::debug; use crate::{util::read_and_parse, Body, BodyChunk, BodyErrorReason}; -use fluke_buffet::{Piece, PieceList, RollMut}; +use fluke_buffet::{PieceCore, PieceList, RollMut}; use fluke_maybe_uring::io::{ReadOwned, WriteOwned}; /// An HTTP/1.1 body, either chunked or content-length. @@ -266,7 +266,7 @@ pub(crate) async fn write_h1_body( pub(crate) async fn write_h1_body_chunk( transport: &mut impl WriteOwned, - chunk: Piece, + chunk: PieceCore, mode: BodyWriteMode, ) -> eyre::Result<()> { match mode { diff --git a/crates/fluke/src/h1/encode.rs b/crates/fluke/src/h1/encode.rs index 8451915d..5db6f381 100644 --- a/crates/fluke/src/h1/encode.rs +++ b/crates/fluke/src/h1/encode.rs @@ -7,7 +7,7 @@ use crate::{ types::{Headers, Request, Response}, Encoder, }; -use fluke_buffet::{Piece, PieceList, RollMut}; +use fluke_buffet::{PieceCore, PieceList, RollMut}; use fluke_maybe_uring::io::WriteOwned; use super::body::{write_h1_body_chunk, write_h1_body_end, BodyWriteMode}; @@ -168,7 +168,11 @@ where } // TODO: move `mode` into `H1Encoder`? we don't need it for h2 - async fn write_body_chunk(&mut self, chunk: Piece, mode: BodyWriteMode) -> eyre::Result<()> { + async fn write_body_chunk( + &mut self, + chunk: PieceCore, + mode: BodyWriteMode, + ) -> eyre::Result<()> { // TODO: inline write_h1_body_chunk(&mut self.transport_w, chunk, mode).await } diff --git a/crates/fluke/src/h2/body.rs b/crates/fluke/src/h2/body.rs index 2ac7d52d..c749cfb0 100644 --- a/crates/fluke/src/h2/body.rs +++ b/crates/fluke/src/h2/body.rs @@ -1,10 +1,10 @@ use tokio::sync::mpsc; use crate::{Body, BodyChunk, Headers}; -use fluke_buffet::Piece; +use fluke_buffet::PieceCore; pub(crate) enum PieceOrTrailers { - Piece(Piece), + Piece(PieceCore), Trailers(Box), } diff --git a/crates/fluke/src/h2/encode.rs b/crates/fluke/src/h2/encode.rs index 30bb5e04..c5f60cd4 100644 --- a/crates/fluke/src/h2/encode.rs +++ b/crates/fluke/src/h2/encode.rs @@ -51,7 +51,7 @@ impl Encoder for H2Encoder { // TODO: BodyWriteMode is not relevant for h2 async fn write_body_chunk( &mut self, - chunk: fluke_buffet::Piece, + chunk: fluke_buffet::PieceCore, _mode: BodyWriteMode, ) -> eyre::Result<()> { assert!(matches!(self.state, EncoderState::ExpectResponseBody)); diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index 2a2f4249..975792a5 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -526,7 +526,7 @@ impl ServerContext { // FIXME: this isn't great, because, due to biased polling, body pieces can pile up. // when we've collected enough pieces for max frame size, we should really send them. - outgoing.pieces.push_back(chunk); + outgoing.pieces.push_back(Piece::Full { core: chunk }); debug!(stream_id = %ev.stream_id, pending_chunks = %outgoing.pieces.len(), "got a body chunk"); self.state.streams_with_pending_data.insert(ev.stream_id); diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index 949fbe93..401f7d91 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -3,7 +3,7 @@ use std::{ fmt, }; -use fluke_buffet::Piece; +use fluke_buffet::{Piece, PieceCore}; use tokio::sync::Notify; use crate::Response; @@ -378,7 +378,7 @@ pub(crate) struct H2Event { pub(crate) enum H2EventPayload { Headers(Response), - BodyChunk(Piece), + BodyChunk(PieceCore), BodyEnd, } diff --git a/crates/fluke/src/responder.rs b/crates/fluke/src/responder.rs index b4587fcf..4b5bf99e 100644 --- a/crates/fluke/src/responder.rs +++ b/crates/fluke/src/responder.rs @@ -1,7 +1,7 @@ use http::header; use crate::{h1::body::BodyWriteMode, Body, BodyChunk, Headers, HeadersExt, Response}; -use fluke_buffet::Piece; +use fluke_buffet::PieceCore; pub trait ResponseState {} @@ -118,7 +118,7 @@ where { /// Send a response body chunk. Errors out if sending more than the /// announced content-length. - pub async fn write_chunk(&mut self, chunk: Piece) -> eyre::Result<()> { + pub async fn write_chunk(&mut self, chunk: PieceCore) -> eyre::Result<()> { self.encoder.write_body_chunk(chunk, self.state.mode).await } @@ -158,7 +158,8 @@ where #[allow(async_fn_in_trait)] // we never require Send pub trait Encoder { async fn write_response(&mut self, res: Response) -> eyre::Result<()>; - async fn write_body_chunk(&mut self, chunk: Piece, mode: BodyWriteMode) -> eyre::Result<()>; + async fn write_body_chunk(&mut self, chunk: PieceCore, mode: BodyWriteMode) + -> eyre::Result<()>; async fn write_body_end(&mut self, mode: BodyWriteMode) -> eyre::Result<()>; async fn write_trailers(&mut self, trailers: Box) -> eyre::Result<()>; } diff --git a/crates/fluke/src/types/mod.rs b/crates/fluke/src/types/mod.rs index a834735a..fee9b56b 100644 --- a/crates/fluke/src/types/mod.rs +++ b/crates/fluke/src/types/mod.rs @@ -3,7 +3,7 @@ use std::fmt::{self, Debug}; use http::{StatusCode, Uri, Version}; use tracing::debug; -use fluke_buffet::Piece; +use fluke_buffet::PieceCore; mod headers; pub use headers::*; @@ -97,7 +97,7 @@ impl Response { /// A body chunk pub enum BodyChunk { - Chunk(Piece), + Chunk(PieceCore), /// The body finished, and it matched the announced content-length, /// or we were using a framed protocol diff --git a/test-crates/fluke-curl-tests/tests/integration_test.rs b/test-crates/fluke-curl-tests/tests/integration_test.rs index fe894fe2..47c013eb 100644 --- a/test-crates/fluke-curl-tests/tests/integration_test.rs +++ b/test-crates/fluke-curl-tests/tests/integration_test.rs @@ -3,7 +3,7 @@ mod helpers; use bytes::BytesMut; use curl::easy::{Easy, HttpVersion, List}; use fluke::{ - buffet::{Piece, RollMut}, + buffet::{Piece, PieceCore, RollMut}, h1, h2, maybe_uring::io::{ChanRead, ChanWrite, IntoHalves}, Body, BodyChunk, Encoder, ExpectResponseHeaders, Headers, HeadersExt, Method, Request, @@ -974,7 +974,9 @@ impl Body for SampleBody { async fn next_chunk(&mut self) -> eyre::Result { let c = match self.chunks_remain { 0 => BodyChunk::Done { trailers: None }, - _ => BodyChunk::Chunk(Piece::Vec(b"this is a big chunk".to_vec().repeat(256))), + _ => BodyChunk::Chunk(PieceCore::Vec(Rc::new( + b"this is a big chunk".to_vec().repeat(256), + ))), }; if let Some(remain) = self.chunks_remain.checked_sub(1) { From 1c4c2cd6edae00cdd3a95a0d294dcba1d2f04dc9 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 14:39:04 +0100 Subject: [PATCH 14/25] wip Piece --- crates/fluke-buffet/src/piece.rs | 35 ++++++--- crates/fluke-maybe-uring/src/io.rs | 16 ++-- crates/fluke/src/h1/body.rs | 6 +- crates/fluke/src/h1/encode.rs | 36 ++++----- crates/fluke/src/h2/server.rs | 117 ++++++++++++++++++----------- crates/fluke/src/h2/types.rs | 6 ++ 6 files changed, 131 insertions(+), 85 deletions(-) diff --git a/crates/fluke-buffet/src/piece.rs b/crates/fluke-buffet/src/piece.rs index 63bfc3ec..a8ee935c 100644 --- a/crates/fluke-buffet/src/piece.rs +++ b/crates/fluke-buffet/src/piece.rs @@ -1,6 +1,6 @@ //! Types for performing vectored I/O. -use std::{fmt, ops::Deref, rc::Rc, str::Utf8Error}; +use std::{collections::VecDeque, fmt, ops::Deref, rc::Rc, str::Utf8Error}; use fluke_maybe_uring::buf::IoBuf; use http::header::HeaderName; @@ -250,18 +250,29 @@ pub struct PieceList { // ownership of it. // // we could however do our own memory pooling. - pieces: Vec, + pieces: VecDeque, } impl PieceList { - /// Add a single chunk to the list - pub fn push(&mut self, chunk: impl Into) { - self.pieces.push(chunk.into()); + /// Add a single chunk to the back of the list + pub fn push_back(&mut self, chunk: impl Into) { + self.pieces.push_back(chunk.into()); } - /// Add a single chunk to the list and return self - pub fn with(mut self, chunk: impl Into) -> Self { - self.push(chunk); + /// Add a single chunk to the back list and return self + pub fn followed_by(mut self, chunk: impl Into) -> Self { + self.push_back(chunk); + self + } + + /// Add a single chunk to the front of the list + pub fn push_front(&mut self, chunk: impl Into) { + self.pieces.push_front(chunk.into()); + } + + /// Add a single chunk to the front of the list and return self + pub fn preceded_by(mut self, chunk: impl Into) -> Self { + self.push_front(chunk); self } @@ -282,18 +293,18 @@ impl PieceList { self.pieces.clear(); } - pub fn into_vec(self) -> Vec { + pub fn into_vec_deque(self) -> VecDeque { self.pieces } } -impl From> for PieceList { - fn from(chunks: Vec) -> Self { +impl From> for PieceList { + fn from(chunks: VecDeque) -> Self { Self { pieces: chunks } } } -impl From for Vec { +impl From for VecDeque { fn from(list: PieceList) -> Self { list.pieces } diff --git a/crates/fluke-maybe-uring/src/io.rs b/crates/fluke-maybe-uring/src/io.rs index cd1fd615..24bb3cc1 100644 --- a/crates/fluke-maybe-uring/src/io.rs +++ b/crates/fluke-maybe-uring/src/io.rs @@ -1,4 +1,4 @@ -use std::net::Shutdown; +use std::{collections::VecDeque, net::Shutdown}; use crate::{ buf::{IoBuf, IoBufMut}, @@ -45,15 +45,15 @@ pub trait WriteOwned { /// Write a list of buffers, taking ownership for the duration of the write. /// Might perform a partial write, see [WriteOwned::writev_all] - async fn writev(&mut self, list: Vec) -> BufResult> { - let mut out_list = Vec::with_capacity(list.len()); + async fn writev(&mut self, list: VecDeque) -> BufResult> { + let mut out_list = VecDeque::with_capacity(list.len()); let mut list = list.into_iter(); let mut total = 0; while let Some(buf) = list.next() { let buf_len = buf.bytes_init(); let (res, buf) = self.write(buf).await; - out_list.push(buf); + out_list.push_back(buf); match res { Ok(0) => { @@ -86,10 +86,12 @@ pub trait WriteOwned { } /// Write a list of buffers, re-trying the write if the kernel does a partial write. - async fn writev_all(&mut self, list: impl Into>) -> std::io::Result<()> { - // FIXME: converting into a `Vec` and _then_ into an iterator is silly, + async fn writev_all(&mut self, list: impl Into>) -> std::io::Result<()> { + // FIXME: converting into a `VecDeque` and _then_ into an iterator is silly, // we can probably find a better function signature here. - let mut list: Vec<_> = list.into().into_iter().map(BufOrSlice::Buf).collect(); + let mut list: VecDeque<_> = list.into().into_iter().map(BufOrSlice::Buf).collect(); + // TODO: figure out if `Piece::Slice` is redundant with `BufOrSlice`. Probably is, + // but they're different abstraction levels, so. while !list.is_empty() { let res; diff --git a/crates/fluke/src/h1/body.rs b/crates/fluke/src/h1/body.rs index fe76b7b1..87e119a0 100644 --- a/crates/fluke/src/h1/body.rs +++ b/crates/fluke/src/h1/body.rs @@ -274,9 +274,9 @@ pub(crate) async fn write_h1_body_chunk( transport .writev_all( PieceList::default() - .with(format!("{:x}\r\n", chunk.len()).into_bytes()) - .with(chunk) - .with("\r\n"), + .followed_by(format!("{:x}\r\n", chunk.len()).into_bytes()) + .followed_by(chunk) + .followed_by("\r\n"), ) .await?; } diff --git a/crates/fluke/src/h1/encode.rs b/crates/fluke/src/h1/encode.rs index 5db6f381..91cb9987 100644 --- a/crates/fluke/src/h1/encode.rs +++ b/crates/fluke/src/h1/encode.rs @@ -17,39 +17,39 @@ pub(crate) fn encode_request( list: &mut PieceList, out_scratch: &mut RollMut, ) -> eyre::Result<()> { - list.push(req.method.into_chunk()); - list.push(" "); + list.push_back(req.method.into_chunk()); + list.push_back(" "); assert_eq!(out_scratch.len(), 0); out_scratch.write_all(req.uri.path().as_bytes())?; - list.push(out_scratch.take_all()); + list.push_back(out_scratch.take_all()); match req.version { - Version::HTTP_10 => list.push(" HTTP/1.0\r\n"), - Version::HTTP_11 => list.push(" HTTP/1.1\r\n"), + Version::HTTP_10 => list.push_back(" HTTP/1.0\r\n"), + Version::HTTP_11 => list.push_back(" HTTP/1.1\r\n"), _ => return Err(eyre::eyre!("unsupported HTTP version {:?}", req.version)), } // TODO: if `host` isn't set, set from request uri? which should // take precedence here? encode_headers(req.headers, list)?; - list.push("\r\n"); + list.push_back("\r\n"); Ok(()) } fn encode_response(res: Response, list: &mut PieceList) -> eyre::Result<()> { match res.version { - Version::HTTP_10 => list.push(&b"HTTP/1.0 "[..]), - Version::HTTP_11 => list.push(&b"HTTP/1.1 "[..]), + Version::HTTP_10 => list.push_back(&b"HTTP/1.0 "[..]), + Version::HTTP_11 => list.push_back(&b"HTTP/1.1 "[..]), _ => return Err(eyre::eyre!("unsupported HTTP version {:?}", res.version)), } - list.push(encode_status_code(res.status)); - list.push(" "); - list.push(res.status.canonical_reason().unwrap_or("Unknown")); - list.push("\r\n"); + list.push_back(encode_status_code(res.status)); + list.push_back(" "); + list.push_back(res.status.canonical_reason().unwrap_or("Unknown")); + list.push_back("\r\n"); encode_headers(res.headers, list)?; - list.push("\r\n"); + list.push_back("\r\n"); Ok(()) } @@ -59,19 +59,19 @@ pub(crate) fn encode_headers(headers: Headers, list: &mut PieceList) -> eyre::Re match name { Some(name) => { last_header_name = Some(name.clone()); - list.push(name); + list.push_back(name); } None => { let name = match last_header_name { Some(ref name) => name.clone(), None => unreachable!("HeaderMap's IntoIter violated its contract"), }; - list.push(name); + list.push_back(name); } } - list.push(": "); - list.push(value); - list.push("\r\n"); + list.push_back(": "); + list.push_back(value); + list.push_back("\r\n"); } Ok(()) diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index 975792a5..2cf804ca 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -166,7 +166,7 @@ impl ServerContext { FrameType::Settings(Default::default()), StreamId::CONNECTION, ); - self.write_frame(frame, payload).await?; + self.write_frame(frame, PieceList::default().followed_by(payload)).await?; } let mut goaway_err: Option = None; @@ -244,7 +244,7 @@ impl ServerContext { })?; let frame = Frame::new(FrameType::GoAway, StreamId::CONNECTION); - self.write_frame(frame, payload).await?; + self.write_frame(frame, PieceList::default().followed_by(payload)).await?; } Ok(()) @@ -397,7 +397,7 @@ impl ServerContext { for id in eof_streams { let flags = DataFlags::EndStream; let frame = Frame::new(FrameType::Data(flags.into()), id); - self.write_frame(frame, Roll::empty()).await?; + self.write_frame(frame, PieceList::default()).await?; } if self.state.outgoing_capacity == 0 { @@ -427,43 +427,69 @@ impl ServerContext { continue; } - // how much data do we have available? - let outg_len: u32 = outgoing - .pieces - .iter() - .map(|p| p.len()) - .sum::() - .try_into() - .unwrap_or(u32::MAX); - - // how much data are we allowed to write? - let conn_cap = self.state.outgoing_capacity; - let strm_cap = outgoing.capacity; - let max_fram = self.state.peer_settings.max_frame_size; - - let write_size = conn_cap.min(strm_cap).min(max_fram).min(outg_len); - - debug!(%outg_len, %conn_cap, %strm_cap, %max_fram, %write_size, "ready to write"); - if outg_len == write_size { - // yay we can just send everything - // TODO: send a PieceList and do a single write instead - // TODO: actually respect max frame size - - let mut pieces = std::mem::take(&mut outgoing.pieces); - let eof = outgoing.eof; - let num_pieces = pieces.len(); - - for (i, piece) in pieces.drain(..).enumerate() { - let flags: BitFlags = if eof && i == num_pieces - 1 { - DataFlags::EndStream.into() - } else { - Default::default() + let max_fram = self.state.peer_settings.max_frame_size as usize; + debug!(conn_cap = %self.state.outgoing_capacity, strm_cap = %outgoing.capacity, %max_fram, "ready to write"); + + let capacity = self.state.outgoing_capacity.min(outgoing.capacity) as usize; + // bytes written this turn, possibly over multiple frames + let mut total_bytes_written = 0; + + while total_bytes_written < capacity { + // send as much data as we can, respecting max frame size and + // connection / stream capacity + let mut plist = PieceList::default(); + let mut plist_byte_len = 0; + + 'pop_pieces: loop { + let piece = match outgoing.pieces.pop_front() { + None => break 'pop_pieces, + Some(piece) => piece, }; - let frame = Frame::new(FrameType::Data(flags.into()), id); - self.write_frame(frame, piece).await?; + + // do we need to split the piece because we don't have + // enough capacity left / we hit the max frame size? + let piece_byte_len = piece.len(); + + enum Outcome { + Done, + KeepAccumulating, + } + + let outcome: Outcome; + let write_size: usize; + + let fram_size_if_full_piece = plist_byte_len + piece_byte_len; + if fram_size_if_full_piece > max_fram { + // we can't fit this piece in the current frame, so + // we have to split it + write_size = max_fram - plist_byte_len; + let (written, requeued) = piece.split_at(write_size); + outcome = Outcome::Done; + + plist.push_back(written); + outgoing.pieces.push_front(requeued); // thx johann + } else { + // we can write the full piece + write_size = piece_byte_len; + outcome = Outcome::KeepAccumulating; + + plist.push_back(piece); + } + + total_bytes_written += write_size; + plist_byte_len += write_size; + + if let Outcome::Done = outcome { + break 'pop_pieces; + } } - } else { - todo!("handle partial writes") + + let mut flags: BitFlags = Default::default(); + if outgoing.is_eof() { + flags |= DataFlags::EndStream; + } + let frame = Frame::new(FrameType::Data(flags.into()), id); + self.write_frame(frame, plist); } } @@ -565,10 +591,8 @@ impl ServerContext { async fn write_frame( &mut self, mut frame: Frame, - payload: impl Into, + mut payload: PieceList, ) -> Result<(), H2ConnectionError> { - let payload = payload.into(); - match &frame.frame_type { FrameType::Data(flags) => { let mut ss = match self.state.streams.entry(frame.stream_id) { @@ -662,7 +686,7 @@ impl ServerContext { } else { trace!("Writing frame with payload"); self.transport_w - .writev_all(PieceList::default().with(frame_roll).with(payload)) + .writev_all(payload.preceded_by(frame_roll)) .await .map_err(H2ConnectionError::WriteError)?; } @@ -928,7 +952,8 @@ impl ServerContext { FrameType::Settings(SettingsFlags::Ack.into()), StreamId::CONNECTION, ); - self.write_frame(frame, Roll::empty()).await?; + self.write_frame(frame, PieceList::default().(Roll::empty())) + .await?; debug!("Acknowledged peer settings"); } } @@ -955,7 +980,8 @@ impl ServerContext { let flags = PingFlags::Ack.into(); let frame = Frame::new(FrameType::Ping(flags), StreamId::CONNECTION) .with_len(payload.len() as u32); - self.write_frame(frame, payload).await?; + self.write_frame(frame, PieceList::default().followed_by(payload)) + .await?; } FrameType::GoAway => { if frame.stream_id != StreamId::CONNECTION { @@ -1068,7 +1094,8 @@ impl ServerContext { let frame = Frame::new(FrameType::RstStream, stream_id) .with_len((payload.len()).try_into().unwrap()); - self.write_frame(frame, payload).await?; + self.write_frame(frame, PieceList::default().followed_by(payload)) + .await?; Ok(()) } diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index 401f7d91..3ebab39f 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -182,9 +182,15 @@ pub(crate) struct StreamOutgoing { } impl StreamOutgoing { + #[inline(always)] pub(crate) fn is_empty(&self) -> bool { self.pieces.is_empty() } + + #[inline(always)] + pub(crate) fn is_eof(&self) -> bool { + self.eof && self.is_empty() + } } #[derive(Debug, thiserror::Error)] From 5e81e878531e539260e1e1ababd6b5a8f8ba0d5c Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 15:08:05 +0100 Subject: [PATCH 15/25] curl tests work! partial writes work. --- crates/fluke/src/h2/server.rs | 85 +++++++++++-------- .../tests/integration_test.rs | 2 +- 2 files changed, 51 insertions(+), 36 deletions(-) diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index 2cf804ca..ec8acd56 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -166,7 +166,8 @@ impl ServerContext { FrameType::Settings(Default::default()), StreamId::CONNECTION, ); - self.write_frame(frame, PieceList::default().followed_by(payload)).await?; + self.write_frame(frame, PieceList::default().followed_by(payload)) + .await?; } let mut goaway_err: Option = None; @@ -244,7 +245,8 @@ impl ServerContext { })?; let frame = Frame::new(FrameType::GoAway, StreamId::CONNECTION); - self.write_frame(frame, PieceList::default().followed_by(payload)).await?; + self.write_frame(frame, PieceList::default().followed_by(payload)) + .await?; } Ok(()) @@ -414,7 +416,11 @@ impl ServerContext { .copied() .collect(); - for id in streams_with_pending_data { + // this vec exists for borrow-checker reasons: we can't + // borrow self mutably twice in 'each_stream + let mut frames = vec![]; + + 'each_stream: for id in streams_with_pending_data { let outgoing = self .state .streams @@ -424,7 +430,7 @@ impl ServerContext { if outgoing.is_empty() { not_pending.insert(id); - continue; + continue 'each_stream; } let max_fram = self.state.peer_settings.max_frame_size as usize; @@ -434,63 +440,71 @@ impl ServerContext { // bytes written this turn, possibly over multiple frames let mut total_bytes_written = 0; - while total_bytes_written < capacity { + 'queue_frames: while total_bytes_written < capacity { // send as much data as we can, respecting max frame size and // connection / stream capacity let mut plist = PieceList::default(); - let mut plist_byte_len = 0; + let mut frame_len = 0; - 'pop_pieces: loop { + 'build_frame: loop { let piece = match outgoing.pieces.pop_front() { - None => break 'pop_pieces, + None => break 'build_frame, Some(piece) => piece, }; // do we need to split the piece because we don't have // enough capacity left / we hit the max frame size? - let piece_byte_len = piece.len(); - - enum Outcome { - Done, - KeepAccumulating, - } + let piece_len = piece.len(); + debug!(%piece_len, "popped a piece"); - let outcome: Outcome; - let write_size: usize; - - let fram_size_if_full_piece = plist_byte_len + piece_byte_len; + let fram_size_if_full_piece = frame_len + piece_len; if fram_size_if_full_piece > max_fram { // we can't fit this piece in the current frame, so // we have to split it - write_size = max_fram - plist_byte_len; + let write_size = max_fram - frame_len; + frame_len += write_size; let (written, requeued) = piece.split_at(write_size); - outcome = Outcome::Done; + debug!(written_len = %written.len(), requeued_len = %requeued.len(), "splitting piece"); plist.push_back(written); - outgoing.pieces.push_front(requeued); // thx johann + outgoing.pieces.push_front(requeued); + + break 'build_frame; } else { // we can write the full piece - write_size = piece_byte_len; - outcome = Outcome::KeepAccumulating; + let write_size = piece_len; + frame_len += write_size; plist.push_back(piece); } - - total_bytes_written += write_size; - plist_byte_len += write_size; - - if let Outcome::Done = outcome { - break 'pop_pieces; - } } let mut flags: BitFlags = Default::default(); - if outgoing.is_eof() { + let is_eof = outgoing.is_eof(); + if is_eof { flags |= DataFlags::EndStream; + } else { + if frame_len == 0 { + // we've sent all the data we can for this stream + break 'queue_frames; + } } + let frame = Frame::new(FrameType::Data(flags.into()), id); - self.write_frame(frame, plist); + debug!(?frame, %frame_len, "queuing"); + frames.push((frame, plist)); + total_bytes_written += frame_len; + + if is_eof { + break 'queue_frames; + } } + + not_pending.insert(id); + } + + for (frame, plist) in frames { + self.write_frame(frame, plist).await?; } for id in not_pending { @@ -527,7 +541,8 @@ impl ServerContext { .map_err(H2ConnectionError::WriteError)?; let payload = self.out_scratch.take_all(); - self.write_frame(frame, payload).await?; + self.write_frame(frame, PieceList::default().followed_by(payload)) + .await?; } H2EventPayload::BodyChunk(chunk) => { let outgoing = match self @@ -591,7 +606,7 @@ impl ServerContext { async fn write_frame( &mut self, mut frame: Frame, - mut payload: PieceList, + payload: PieceList, ) -> Result<(), H2ConnectionError> { match &frame.frame_type { FrameType::Data(flags) => { @@ -952,7 +967,7 @@ impl ServerContext { FrameType::Settings(SettingsFlags::Ack.into()), StreamId::CONNECTION, ); - self.write_frame(frame, PieceList::default().(Roll::empty())) + self.write_frame(frame, PieceList::default().followed_by(Roll::empty())) .await?; debug!("Acknowledged peer settings"); } diff --git a/test-crates/fluke-curl-tests/tests/integration_test.rs b/test-crates/fluke-curl-tests/tests/integration_test.rs index 47c013eb..a7933d5b 100644 --- a/test-crates/fluke-curl-tests/tests/integration_test.rs +++ b/test-crates/fluke-curl-tests/tests/integration_test.rs @@ -3,7 +3,7 @@ mod helpers; use bytes::BytesMut; use curl::easy::{Easy, HttpVersion, List}; use fluke::{ - buffet::{Piece, PieceCore, RollMut}, + buffet::{PieceCore, RollMut}, h1, h2, maybe_uring::io::{ChanRead, ChanWrite, IntoHalves}, Body, BodyChunk, Encoder, ExpectResponseHeaders, Headers, HeadersExt, Method, Request, From ae8ae1cf87429462ab2fae4867916d5ba95265bc Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 15:12:18 +0100 Subject: [PATCH 16/25] Update deps in fluke-h2spec --- test-crates/fluke-h2spec/Cargo.lock | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test-crates/fluke-h2spec/Cargo.lock b/test-crates/fluke-h2spec/Cargo.lock index 3bf224b5..8ad1cf9c 100644 --- a/test-crates/fluke-h2spec/Cargo.lock +++ b/test-crates/fluke-h2spec/Cargo.lock @@ -61,9 +61,9 @@ checksum = "ed570934406eb16438a4e976b1b4500774099c13b8cb96eec99f620f05090ddf" [[package]] name = "bytemuck" -version = "1.14.3" +version = "1.15.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a2ef034f05691a48569bd920a96c81b9d91bbad1ab5ac7c4616c1f6ef36cb79f" +checksum = "5d6d68c57235a3a081186990eca2867354726650f42f7516ca50c28d6281fd15" [[package]] name = "byteorder" @@ -691,18 +691,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.57" +version = "1.0.58" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e45bcbe8ed29775f228095caf2cd67af7a4ccf756ebff23a306bf3e8b47b24b" +checksum = "03468839009160513471e86a034bb2c5c0e4baae3b43f79ffc55c4a5427b3297" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.57" +version = "1.0.58" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a953cb265bef375dae3de6663da4d3804eee9682ea80d8e2542529b73c531c81" +checksum = "c61f3ba182994efc43764a46c018c347bc492c79f024e705f46567b418f6d4f7" dependencies = [ "proc-macro2", "quote", From 45b0db6c5366c8c39e975611774a50433c123cc1 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 15:17:45 +0100 Subject: [PATCH 17/25] do flow control accounting for headers as well --- crates/fluke/src/h2/server.rs | 140 +++++++++++++++++++--------------- 1 file changed, 78 insertions(+), 62 deletions(-) diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index ec8acd56..fd107643 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -608,76 +608,92 @@ impl ServerContext { mut frame: Frame, payload: PieceList, ) -> Result<(), H2ConnectionError> { - match &frame.frame_type { - FrameType::Data(flags) => { - let mut ss = match self.state.streams.entry(frame.stream_id) { - std::collections::hash_map::Entry::Occupied(entry) => entry, - std::collections::hash_map::Entry::Vacant(_) => unreachable!( - "writing DATA frame for non-existent stream, this should never happen" - ), - }; + struct FlowControlledFrameInfo { + len: usize, + end_stream: bool, + } - // update stream flow control window - { - let outgoing = match ss.get_mut().outgoing_mut() { - Some(og) => og, - None => unreachable!("writing DATA frame for stream in the wrong state"), - }; - let payload_len: u32 = payload.len().try_into().unwrap(); - if let Some(next_cap) = outgoing.capacity.checked_sub(payload_len) { - outgoing.capacity = next_cap; - } else { - unreachable!( - "should never write a frame that makes the stream capacity negative" - ) - } + let flow_controlled_frame_info = match &frame.frame_type { + FrameType::Data(flags) => Some(FlowControlledFrameInfo { + len: payload.len(), + end_stream: flags.contains(DataFlags::EndStream), + }), + FrameType::Headers(flags) => Some(FlowControlledFrameInfo { + len: payload.len(), + end_stream: flags.contains(HeadersFlags::EndStream), + }), + FrameType::Settings(_) => { + // TODO: keep track of whether our new settings have been acknowledged + None + } + _ => { + // muffin. + None + } + }; + + if let Some(flow_controlled_frame_info) = flow_controlled_frame_info { + let mut ss = match self.state.streams.entry(frame.stream_id) { + std::collections::hash_map::Entry::Occupied(entry) => entry, + std::collections::hash_map::Entry::Vacant(_) => unreachable!( + "writing DATA frame for non-existent stream, this should never happen" + ), + }; + + // update stream flow control window + { + let outgoing = match ss.get_mut().outgoing_mut() { + Some(og) => og, + None => unreachable!("writing DATA frame for stream in the wrong state"), + }; + let payload_len: u32 = payload.len().try_into().unwrap(); + if let Some(next_cap) = outgoing.capacity.checked_sub(payload_len) { + outgoing.capacity = next_cap; + } else { + unreachable!( + "should never write a frame that makes the stream capacity negative" + ) } + } - // now update connection flow control window - { - let payload_len: u32 = payload.len().try_into().unwrap(); - if let Some(next_cap) = self.state.outgoing_capacity.checked_sub(payload_len) { - self.state.outgoing_capacity = next_cap; - } else { - unreachable!( - "should never write a frame that makes the connection capacity negative" - ) - } + // now update connection flow control window + { + let payload_len: u32 = payload.len().try_into().unwrap(); + if let Some(next_cap) = self.state.outgoing_capacity.checked_sub(payload_len) { + self.state.outgoing_capacity = next_cap; + } else { + unreachable!( + "should never write a frame that makes the connection capacity negative" + ) } + } - if flags.contains(DataFlags::EndStream) { - // we won't be sending any more data on this stream - self.state - .streams_with_pending_data - .remove(&frame.stream_id); - - match ss.get_mut() { - StreamState::Open { .. } => { - let incoming = match std::mem::take(ss.get_mut()) { - StreamState::Open { incoming, .. } => incoming, - _ => unreachable!(), - }; - // this avoid having to re-insert the stream in the map - *ss.get_mut() = StreamState::HalfClosedLocal { incoming }; - } - _ => { - // transition to closed - ss.remove(); - debug!( - "Closed stream {} (wrote data w/EndStream), now have {} streams", - frame.stream_id, - self.state.streams.len() - ); - } + if flow_controlled_frame_info.end_stream { + // we won't be sending any more data on this stream + self.state + .streams_with_pending_data + .remove(&frame.stream_id); + + match ss.get_mut() { + StreamState::Open { .. } => { + let incoming = match std::mem::take(ss.get_mut()) { + StreamState::Open { incoming, .. } => incoming, + _ => unreachable!(), + }; + // this avoid having to re-insert the stream in the map + *ss.get_mut() = StreamState::HalfClosedLocal { incoming }; + } + _ => { + // transition to closed + ss.remove(); + debug!( + "Closed stream {} (wrote data w/EndStream), now have {} streams", + frame.stream_id, + self.state.streams.len() + ); } } } - FrameType::Settings(_) => { - // TODO: keep track of whether our new settings have been acknowledged - } - _ => { - // muffin. - } } // TODO: enforce max_frame_size from the peer settings, not just u32::max From de783e57521db095f04a28642f5ff8fc3d44993f Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 16:03:38 +0100 Subject: [PATCH 18/25] wip header flow control --- crates/fluke-buffet/src/piece.rs | 9 ++ crates/fluke/src/h2/server.rs | 223 +++++++++++++++++-------------- crates/fluke/src/h2/types.rs | 89 ++++++++++-- 3 files changed, 210 insertions(+), 111 deletions(-) diff --git a/crates/fluke-buffet/src/piece.rs b/crates/fluke-buffet/src/piece.rs index a8ee935c..45b2b865 100644 --- a/crates/fluke-buffet/src/piece.rs +++ b/crates/fluke-buffet/src/piece.rs @@ -21,6 +21,15 @@ pub enum Piece { }, } +impl Piece { + /// Returns an empty piece + pub fn empty() -> Self { + Self::Full { + core: PieceCore::Static(&[]), + } + } +} + #[derive(Clone)] pub enum PieceCore { Static(&'static [u8]), diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index fd107643..8da773f1 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -32,7 +32,7 @@ use crate::{ }, types::{ ConnState, H2ConnectionError, H2Event, H2EventPayload, H2StreamError, - HeadersOrTrailers, StreamOutgoing, StreamState, + HeadersOrTrailers, HeadersOutgoing, StreamOutgoing, StreamState, }, }, util::read_and_parse, @@ -87,28 +87,6 @@ pub(crate) struct ServerContext { ev_rx: mpsc::Receiver, } -impl ConnState { - fn eof_streams(&self) -> HashSet { - self.streams_with_pending_data - .iter() - .copied() - .filter_map(|id| { - self.streams - .get(&id) - .and_then(|ss| ss.outgoing_ref()) - .map(|og| (id, og)) - }) - .filter_map(|(id, outgoing)| { - if outgoing.eof && outgoing.is_empty() { - Some(id) - } else { - None - } - }) - .collect() - } -} - impl ServerContext { pub(crate) fn new(driver: Rc, state: ConnState, transport_w: W) -> eyre::Result { let mut hpack_dec = fluke_hpack::Decoder::new(); @@ -395,18 +373,6 @@ impl ServerContext { } async fn send_data_maybe(&mut self) -> Result<(), H2ConnectionError> { - let eof_streams: HashSet = self.state.eof_streams(); - for id in eof_streams { - let flags = DataFlags::EndStream; - let frame = Frame::new(FrameType::Data(flags.into()), id); - self.write_frame(frame, PieceList::default()).await?; - } - - if self.state.outgoing_capacity == 0 { - // that's all we can do - return Ok(()); - } - let mut not_pending: HashSet = Default::default(); let streams_with_pending_data: HashSet<_> = self @@ -421,6 +387,11 @@ impl ServerContext { let mut frames = vec![]; 'each_stream: for id in streams_with_pending_data { + if self.state.outgoing_capacity == 0 { + // that's all we can do + break 'each_stream; + } + let outgoing = self .state .streams @@ -428,11 +399,6 @@ impl ServerContext { .and_then(|ss| ss.outgoing_mut()) .expect("stream should not be in streams_with_pending_data if it's already closed / not in an outgoing state"); - if outgoing.is_empty() { - not_pending.insert(id); - continue 'each_stream; - } - let max_fram = self.state.peer_settings.max_frame_size as usize; debug!(conn_cap = %self.state.outgoing_capacity, strm_cap = %outgoing.capacity, %max_fram, "ready to write"); @@ -440,63 +406,106 @@ impl ServerContext { // bytes written this turn, possibly over multiple frames let mut total_bytes_written = 0; - 'queue_frames: while total_bytes_written < capacity { - // send as much data as we can, respecting max frame size and - // connection / stream capacity - let mut plist = PieceList::default(); - let mut frame_len = 0; - - 'build_frame: loop { - let piece = match outgoing.pieces.pop_front() { - None => break 'build_frame, - Some(piece) => piece, - }; + if outgoing.headers.has_more_to_write() { + if matches!(&outgoing.headers, HeadersOutgoing::WaitingForHeaders) { + // shouldn't be pending then should it? + not_pending.insert(id); + continue 'each_stream; + } - // do we need to split the piece because we don't have - // enough capacity left / we hit the max frame size? + 'queue_header_frames: while total_bytes_written < capacity { + let is_continuation = + matches!(&outgoing.headers, HeadersOutgoing::WroteSome(_)); + let piece = outgoing.headers.take_piece(); let piece_len = piece.len(); - debug!(%piece_len, "popped a piece"); - - let fram_size_if_full_piece = frame_len + piece_len; - if fram_size_if_full_piece > max_fram { - // we can't fit this piece in the current frame, so - // we have to split it - let write_size = max_fram - frame_len; - frame_len += write_size; - let (written, requeued) = piece.split_at(write_size); - debug!(written_len = %written.len(), requeued_len = %requeued.len(), "splitting piece"); - plist.push_back(written); - outgoing.pieces.push_front(requeued); + if piece_len > max_fram { + let (written, requeued) = piece.split_at(max_fram); + debug!(written_len = %written.len(), requeued_len = %requeued.len(), "splitting headers"); + let frame_type = if is_continuation { + FrameType::Continuation(Default::default()) + } else { + FrameType::Headers(Default::default()) + }; + outgoing.headers = HeadersOutgoing::WroteSome(requeued); - break 'build_frame; + let frame = Frame::new(frame_type, id); + total_bytes_written += written.len(); + frames.push((frame, written)); } else { - // we can write the full piece - let write_size = piece_len; - frame_len += write_size; + let frame_type = if is_continuation { + FrameType::Continuation( + Default::default() | ContinuationFlags::EndHeaders, + ) + } else { + FrameType::Headers(Default::default() | HeadersFlags::EndHeaders) + }; - plist.push_back(piece); + total_bytes_written += piece_len; + break 'queue_header_frames; } } + } - let mut flags: BitFlags = Default::default(); - let is_eof = outgoing.is_eof(); - if is_eof { - flags |= DataFlags::EndStream; - } else { - if frame_len == 0 { - // we've sent all the data we can for this stream - break 'queue_frames; + if outgoing.body.has_more_to_write() { + 'queue_body_frames: while total_bytes_written < capacity { + // send as much body data as we can, respecting max frame size and + // connection / stream capacity + let mut plist = PieceList::default(); + let mut frame_len = 0; + + 'build_frame: loop { + let piece = match outgoing.body.pop_front() { + None => break 'build_frame, + Some(piece) => piece, + }; + + // do we need to split the piece because we don't have + // enough capacity left / we hit the max frame size? + let piece_len = piece.len(); + debug!(%piece_len, "popped a piece"); + + let fram_size_if_full_piece = frame_len + piece_len; + if fram_size_if_full_piece > max_fram { + // we can't fit this piece in the current frame, so + // we have to split it + let write_size = max_fram - frame_len; + frame_len += write_size; + let (written, requeued) = piece.split_at(write_size); + debug!(written_len = %written.len(), requeued_len = %requeued.len(), "splitting piece"); + + plist.push_back(written); + outgoing.pieces.push_front(requeued); + + break 'build_frame; + } else { + // we can write the full piece + let write_size = piece_len; + frame_len += write_size; + + plist.push_back(piece); + } + } + + let mut flags: BitFlags = Default::default(); + let is_eof = outgoing.has_more_body(); + if is_eof { + flags |= DataFlags::EndStream; + } else { + if frame_len == 0 { + // we've sent all the data we can for this stream + break 'queue_body_frames; + } } - } - let frame = Frame::new(FrameType::Data(flags.into()), id); - debug!(?frame, %frame_len, "queuing"); - frames.push((frame, plist)); - total_bytes_written += frame_len; + let frame = Frame::new(FrameType::Data(flags.into()), id); + debug!(?frame, %frame_len, "queuing"); + frames.push((frame, plist)); + total_bytes_written += frame_len; - if is_eof { - break 'queue_frames; + if is_eof { + break 'queue_body_frames; + } } } @@ -517,6 +526,26 @@ impl ServerContext { async fn handle_event(&mut self, ev: H2Event) -> Result<(), H2ConnectionError> { match ev.payload { H2EventPayload::Headers(res) => { + let outgoing = match self + .state + .streams + .get_mut(&ev.stream_id) + .and_then(|s| s.outgoing_mut()) + { + None => { + // ignore the event then, but at this point we should + // tell the sender to stop sending chunks, which is not + // possible if they all share the same ev_tx + // TODO: make it possible to propagate errors to the sender + return Ok(()); + } + Some(outgoing) => outgoing, + }; + + if outgoing.end_stream { + unreachable!("got headers after the body end event!") + } + let flags = HeadersFlags::EndHeaders; let frame = Frame::new(FrameType::Headers(flags.into()), ev.stream_id); @@ -561,13 +590,13 @@ impl ServerContext { Some(outgoing) => outgoing, }; - if outgoing.eof { + if outgoing.end_stream { unreachable!("got a body chunk after the body end event!") } // FIXME: this isn't great, because, due to biased polling, body pieces can pile up. // when we've collected enough pieces for max frame size, we should really send them. - outgoing.pieces.push_back(Piece::Full { core: chunk }); + outgoing.body.push_back(Piece::Full { core: chunk }); debug!(stream_id = %ev.stream_id, pending_chunks = %outgoing.pieces.len(), "got a body chunk"); self.state.streams_with_pending_data.insert(ev.stream_id); @@ -587,16 +616,16 @@ impl ServerContext { Some(outgoing) => outgoing, }; - if outgoing.eof { + if outgoing.end_stream { panic!("got a body end event after the body end event!") } - debug!(stream_id = %ev.stream_id, pending_chunks = %outgoing.pieces.len(), "got body end"); - outgoing.eof = true; - if outgoing.is_empty() { - // we'll need to send a zero-length data frame - self.state.send_data_maybe.notify_one(); - } + debug!(stream_id = %ev.stream_id, pending_chunks = %outgoing.body.len(), "got body end"); + outgoing.end_stream = true; + + // we'll need to send a zero-length data frame + // TODO: maybe we don't always need to notify here + self.state.send_data_maybe.notify_one(); } } @@ -1359,16 +1388,12 @@ impl ServerContext { tx: piece_tx, }; let outgoing: StreamOutgoing = self.state.mk_stream_outgoing(); - self.state.streams.insert( stream_id, if end_stream { StreamState::HalfClosedRemote { outgoing } } else { - StreamState::Open { - incoming, - outgoing: self.state.mk_stream_outgoing(), - } + StreamState::Open { incoming, outgoing } }, ); debug!( diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index 3ebab39f..06cac08b 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -24,6 +24,9 @@ pub(crate) struct ConnState { /// - an H2Body has been written to, AND /// - the corresponding stream has available capacity /// - the connection has available capacity + /// + /// FIXME: we don't need Notify at all, it uses atomic operations + /// but all we're doing is single-threaded. pub(crate) send_data_maybe: Notify, pub(crate) streams_with_pending_data: HashSet, @@ -57,9 +60,9 @@ impl ConnState { /// create a new [StreamOutgoing] based on our current settings pub(crate) fn mk_stream_outgoing(&self) -> StreamOutgoing { StreamOutgoing { - pieces: Default::default(), + headers: HeadersOutgoing::WaitingForHeaders, + body: BodyOutgoing::StillReceiving(Default::default()), capacity: self.peer_settings.initial_window_size, - eof: false, } } } @@ -169,27 +172,89 @@ impl StreamState { } pub(crate) struct StreamOutgoing { - // list of pieces we need to send out - pub(crate) pieces: VecDeque, + pub(crate) headers: HeadersOutgoing, + pub(crate) body: BodyOutgoing, // window size of the stream, ie. how many bytes // we can send to the receiver before waiting. pub(crate) capacity: u32, +} + +#[derive(Default)] +pub(crate) enum HeadersOutgoing { + // We have not yet sent any headers, and are waiting for the user to send them + WaitingForHeaders, + + // The user gave us headers to send, but we haven't started yet + WroteNone(Piece), - // true if the stream has been closed (ie. all the pieces - // have been sent and the receiver has been notified) - pub(crate) eof: bool, + // We have sent some headers, but not all (we're still sending CONTINUATION frames) + WroteSome(Piece), + + // We've sent everything + #[default] + WroteAll, } -impl StreamOutgoing { +impl HeadersOutgoing { #[inline(always)] - pub(crate) fn is_empty(&self) -> bool { - self.pieces.is_empty() + pub(crate) fn will_receive_more(&self) -> bool { + match self { + HeadersOutgoing::WaitingForHeaders => true, + HeadersOutgoing::WroteNone(_) => true, + HeadersOutgoing::WroteSome(_) => true, + HeadersOutgoing::WroteAll => false, + } } #[inline(always)] - pub(crate) fn is_eof(&self) -> bool { - self.eof && self.is_empty() + pub(crate) fn has_more_to_write(&self) -> bool { + match self { + HeadersOutgoing::WaitingForHeaders => true, + HeadersOutgoing::WroteNone(_) => true, + HeadersOutgoing::WroteSome(_) => true, + HeadersOutgoing::WroteAll => false, + } + } + + #[inline(always)] + pub(crate) fn take_piece(&mut self) -> Piece { + match std::mem::take(self) { + Self::WroteNone(piece) => piece, + Self::WroteSome(piece) => piece, + _ => Piece::empty(), + } + } +} + +pub(crate) enum BodyOutgoing { + /// We are still receiving body pieces from the user + StillReceiving(VecDeque), + + /// We have received all body pieces from the user + DoneReceiving(VecDeque), + + /// We have sent all data to the peer + DoneSending, +} + +impl BodyOutgoing { + #[inline(always)] + pub(crate) fn will_receive_more(&self) -> bool { + match self { + BodyOutgoing::StillReceiving(_) => true, + BodyOutgoing::DoneReceiving(_) => true, + BodyOutgoing::DoneSending => false, + } + } + + #[inline(always)] + pub(crate) fn has_more_to_write(&self) -> bool { + match self { + BodyOutgoing::StillReceiving(_) => true, + BodyOutgoing::DoneReceiving(_) => true, + BodyOutgoing::DoneSending => false, + } } } From cef91bedeb8f21e72dfb815a1842203de7cc579b Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 17:39:27 +0100 Subject: [PATCH 19/25] Headers flow control wip --- crates/fluke-buffet/src/piece.rs | 7 ++ crates/fluke/src/h2/server.rs | 115 +++++++++++++++++-------------- crates/fluke/src/h2/types.rs | 59 +++++++++++++++- 3 files changed, 127 insertions(+), 54 deletions(-) diff --git a/crates/fluke-buffet/src/piece.rs b/crates/fluke-buffet/src/piece.rs index 45b2b865..7e92f2ab 100644 --- a/crates/fluke-buffet/src/piece.rs +++ b/crates/fluke-buffet/src/piece.rs @@ -263,6 +263,13 @@ pub struct PieceList { } impl PieceList { + /// Create a new piece list with a single chunk + pub fn single(piece: impl Into) -> Self { + Self { + pieces: [piece.into()].into(), + } + } + /// Add a single chunk to the back of the list pub fn push_back(&mut self, chunk: impl Into) { self.pieces.push_back(chunk.into()); diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index 8da773f1..bf095b6b 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -31,7 +31,7 @@ use crate::{ HeadersFlags, PingFlags, PrioritySpec, Settings, SettingsFlags, StreamId, }, types::{ - ConnState, H2ConnectionError, H2Event, H2EventPayload, H2StreamError, + BodyOutgoing, ConnState, H2ConnectionError, H2Event, H2EventPayload, H2StreamError, HeadersOrTrailers, HeadersOutgoing, StreamOutgoing, StreamState, }, }, @@ -144,8 +144,7 @@ impl ServerContext { FrameType::Settings(Default::default()), StreamId::CONNECTION, ); - self.write_frame(frame, PieceList::default().followed_by(payload)) - .await?; + self.write_frame(frame, PieceList::single(payload)).await?; } let mut goaway_err: Option = None; @@ -223,8 +222,7 @@ impl ServerContext { })?; let frame = Frame::new(FrameType::GoAway, StreamId::CONNECTION); - self.write_frame(frame, PieceList::default().followed_by(payload)) - .await?; + self.write_frame(frame, PieceList::single(payload)).await?; } Ok(()) @@ -375,6 +373,13 @@ impl ServerContext { async fn send_data_maybe(&mut self) -> Result<(), H2ConnectionError> { let mut not_pending: HashSet = Default::default(); + // this vec exists for borrow-checker reasons: we can't + // borrow self mutably twice in 'each_stream + // TODO: merge those frames! do a single writev_all call! + let mut frames: Vec<(Frame, PieceList)> = vec![]; + + let max_fram = self.state.peer_settings.max_frame_size as usize; + let streams_with_pending_data: HashSet<_> = self .state .streams_with_pending_data @@ -382,10 +387,6 @@ impl ServerContext { .copied() .collect(); - // this vec exists for borrow-checker reasons: we can't - // borrow self mutably twice in 'each_stream - let mut frames = vec![]; - 'each_stream: for id in streams_with_pending_data { if self.state.outgoing_capacity == 0 { // that's all we can do @@ -399,7 +400,6 @@ impl ServerContext { .and_then(|ss| ss.outgoing_mut()) .expect("stream should not be in streams_with_pending_data if it's already closed / not in an outgoing state"); - let max_fram = self.state.peer_settings.max_frame_size as usize; debug!(conn_cap = %self.state.outgoing_capacity, strm_cap = %outgoing.capacity, %max_fram, "ready to write"); let capacity = self.state.outgoing_capacity.min(outgoing.capacity) as usize; @@ -431,17 +431,23 @@ impl ServerContext { let frame = Frame::new(frame_type, id); total_bytes_written += written.len(); - frames.push((frame, written)); + frames.push((frame, PieceList::single(written))); } else { let frame_type = if is_continuation { FrameType::Continuation( - Default::default() | ContinuationFlags::EndHeaders, + BitFlags::::default() + | ContinuationFlags::EndHeaders, ) } else { - FrameType::Headers(Default::default() | HeadersFlags::EndHeaders) + FrameType::Headers( + BitFlags::::default() | HeadersFlags::EndHeaders, + ) }; + let frame = Frame::new(frame_type, id); total_bytes_written += piece_len; + frames.push((frame, PieceList::single(piece))); + break 'queue_header_frames; } } @@ -466,16 +472,19 @@ impl ServerContext { debug!(%piece_len, "popped a piece"); let fram_size_if_full_piece = frame_len + piece_len; - if fram_size_if_full_piece > max_fram { + + let cap_left = capacity - total_bytes_written; + let max_this_fram = max_fram.min(cap_left); + + if fram_size_if_full_piece > max_this_fram { // we can't fit this piece in the current frame, so // we have to split it - let write_size = max_fram - frame_len; - frame_len += write_size; - let (written, requeued) = piece.split_at(write_size); + frame_len += max_this_fram; + let (written, requeued) = piece.split_at(max_this_fram); debug!(written_len = %written.len(), requeued_len = %requeued.len(), "splitting piece"); plist.push_back(written); - outgoing.pieces.push_front(requeued); + outgoing.body.push_front(requeued); break 'build_frame; } else { @@ -488,14 +497,15 @@ impl ServerContext { } let mut flags: BitFlags = Default::default(); - let is_eof = outgoing.has_more_body(); - if is_eof { - flags |= DataFlags::EndStream; - } else { + if outgoing.body.might_receive_more() { if frame_len == 0 { - // we've sent all the data we can for this stream + // the only time we want to send a zero-length frame + // is if we have to send END_STREAM separately from + // the last chunk. break 'queue_body_frames; } + } else { + flags |= DataFlags::EndStream; } let frame = Frame::new(FrameType::Data(flags.into()), id); @@ -503,13 +513,11 @@ impl ServerContext { frames.push((frame, plist)); total_bytes_written += frame_len; - if is_eof { + if flags.contains(DataFlags::EndStream) { break 'queue_body_frames; } } } - - not_pending.insert(id); } for (frame, plist) in frames { @@ -542,20 +550,19 @@ impl ServerContext { Some(outgoing) => outgoing, }; - if outgoing.end_stream { - unreachable!("got headers after the body end event!") + if !matches!(&outgoing.body, BodyOutgoing::StillReceiving(_)) { + unreachable!("got headers too late") } - let flags = HeadersFlags::EndHeaders; - let frame = Frame::new(FrameType::Headers(flags.into()), ev.stream_id); - // TODO: don't allocate so much for headers. all `encode_into` // wants is an `IntoIter`, we can definitely have a custom iterator // that operates on all this instead of using a `Vec`. - // TODO: limit header size + // TODO: enforce max header size let mut headers: Vec<(&[u8], &[u8])> = vec![]; + // TODO: prevent overwriting pseudo-headers, especially :status? headers.push((b":status", res.status.as_str().as_bytes())); + for (name, value) in res.headers.iter() { if name == http::header::TRANSFER_ENCODING { // do not set transfer-encoding: chunked when doing HTTP/2 @@ -570,8 +577,12 @@ impl ServerContext { .map_err(H2ConnectionError::WriteError)?; let payload = self.out_scratch.take_all(); - self.write_frame(frame, PieceList::default().followed_by(payload)) - .await?; + outgoing.headers = HeadersOutgoing::WroteNone(payload.into()); + self.state.streams_with_pending_data.insert(ev.stream_id); + if self.state.outgoing_capacity > 0 && outgoing.capacity > 0 { + // worth revisiting then! + self.state.send_data_maybe.notify_one(); + } } H2EventPayload::BodyChunk(chunk) => { let outgoing = match self @@ -590,14 +601,9 @@ impl ServerContext { Some(outgoing) => outgoing, }; - if outgoing.end_stream { - unreachable!("got a body chunk after the body end event!") - } - // FIXME: this isn't great, because, due to biased polling, body pieces can pile up. // when we've collected enough pieces for max frame size, we should really send them. outgoing.body.push_back(Piece::Full { core: chunk }); - debug!(stream_id = %ev.stream_id, pending_chunks = %outgoing.pieces.len(), "got a body chunk"); self.state.streams_with_pending_data.insert(ev.stream_id); if self.state.outgoing_capacity > 0 && outgoing.capacity > 0 { @@ -616,16 +622,23 @@ impl ServerContext { Some(outgoing) => outgoing, }; - if outgoing.end_stream { - panic!("got a body end event after the body end event!") + match &mut outgoing.body { + BodyOutgoing::StillReceiving(pieces) => { + let pieces = std::mem::take(pieces); + if pieces.len() > 0 { + // we'll need to send a zero-length data frame + self.state.send_data_maybe.notify_one(); + } + outgoing.body = BodyOutgoing::DoneReceiving(pieces); + debug!(stream_id = %ev.stream_id, outgoing_body = ?outgoing.body, "got body end"); + } + BodyOutgoing::DoneReceiving(_) => { + unreachable!("got body end twice") + } + BodyOutgoing::DoneSending => { + unreachable!("got body end after we sent everything") + } } - - debug!(stream_id = %ev.stream_id, pending_chunks = %outgoing.body.len(), "got body end"); - outgoing.end_stream = true; - - // we'll need to send a zero-length data frame - // TODO: maybe we don't always need to notify here - self.state.send_data_maybe.notify_one(); } } @@ -1012,8 +1025,7 @@ impl ServerContext { FrameType::Settings(SettingsFlags::Ack.into()), StreamId::CONNECTION, ); - self.write_frame(frame, PieceList::default().followed_by(Roll::empty())) - .await?; + self.write_frame(frame, PieceList::default()).await?; debug!("Acknowledged peer settings"); } } @@ -1154,8 +1166,7 @@ impl ServerContext { let frame = Frame::new(FrameType::RstStream, stream_id) .with_len((payload.len()).try_into().unwrap()); - self.write_frame(frame, PieceList::default().followed_by(payload)) - .await?; + self.write_frame(frame, PieceList::single(payload)).await?; Ok(()) } diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index 06cac08b..fb211556 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -197,8 +197,9 @@ pub(crate) enum HeadersOutgoing { } impl HeadersOutgoing { + /// It's still possible that we might receive more headers from the user. #[inline(always)] - pub(crate) fn will_receive_more(&self) -> bool { + pub(crate) fn might_receive_more(&self) -> bool { match self { HeadersOutgoing::WaitingForHeaders => true, HeadersOutgoing::WroteNone(_) => true, @@ -238,9 +239,26 @@ pub(crate) enum BodyOutgoing { DoneSending, } +impl fmt::Debug for BodyOutgoing { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + BodyOutgoing::StillReceiving(pieces) => f + .debug_tuple("BodyOutgoing::StillReceiving") + .field(&pieces.len()) + .finish(), + BodyOutgoing::DoneReceiving(pieces) => f + .debug_tuple("BodyOutgoing::DoneReceiving") + .field(&pieces.len()) + .finish(), + BodyOutgoing::DoneSending => f.debug_tuple("BodyOutgoing::DoneSending").finish(), + } + } +} + impl BodyOutgoing { + /// It's still possible for the user to send more data #[inline(always)] - pub(crate) fn will_receive_more(&self) -> bool { + pub(crate) fn might_receive_more(&self) -> bool { match self { BodyOutgoing::StillReceiving(_) => true, BodyOutgoing::DoneReceiving(_) => true, @@ -256,6 +274,43 @@ impl BodyOutgoing { BodyOutgoing::DoneSending => false, } } + + #[inline(always)] + pub(crate) fn pop_front(&mut self) -> Option { + match self { + BodyOutgoing::StillReceiving(pieces) => pieces.pop_front(), + BodyOutgoing::DoneReceiving(pieces) => { + let piece = pieces.pop_front(); + if pieces.is_empty() { + *self = BodyOutgoing::DoneSending; + } + piece + } + BodyOutgoing::DoneSending => None, + } + } + + #[inline(always)] + pub(crate) fn push_front(&mut self, piece: Piece) { + match self { + BodyOutgoing::StillReceiving(pieces) => pieces.push_front(piece), + BodyOutgoing::DoneReceiving(pieces) => pieces.push_front(piece), + BodyOutgoing::DoneSending => { + *self = BodyOutgoing::DoneReceiving([piece].into()); + } + } + } + + #[inline(always)] + pub(crate) fn push_back(&mut self, piece: Piece) { + match self { + BodyOutgoing::StillReceiving(pieces) => pieces.push_back(piece), + BodyOutgoing::DoneReceiving(pieces) => pieces.push_back(piece), + BodyOutgoing::DoneSending => { + unreachable!("received a piece after we were done sending") + } + } + } } #[derive(Debug, thiserror::Error)] From 357337cae371efd870bcd42c996c79a406ff4714 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 17:46:25 +0100 Subject: [PATCH 20/25] well _I_ think 6.9.1 should pass. --- crates/fluke/src/h2/server.rs | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index bf095b6b..52eea73e 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -407,21 +407,31 @@ impl ServerContext { let mut total_bytes_written = 0; if outgoing.headers.has_more_to_write() { + debug!("writing headers..."); + if matches!(&outgoing.headers, HeadersOutgoing::WaitingForHeaders) { + debug!("waiting for headers..."); + // shouldn't be pending then should it? not_pending.insert(id); continue 'each_stream; } 'queue_header_frames: while total_bytes_written < capacity { + debug!(total_bytes_written, capacity, "writing headers..."); + let is_continuation = matches!(&outgoing.headers, HeadersOutgoing::WroteSome(_)); let piece = outgoing.headers.take_piece(); let piece_len = piece.len(); - if piece_len > max_fram { - let (written, requeued) = piece.split_at(max_fram); - debug!(written_len = %written.len(), requeued_len = %requeued.len(), "splitting headers"); + let cap_left = capacity - total_bytes_written; + let max_this_fram = max_fram.min(cap_left); + + if piece_len > max_this_fram { + let write_size = max_this_fram; + let (written, requeued) = piece.split_at(write_size); + debug!(%write_size, requeued_len = %requeued.len(), "splitting headers"); let frame_type = if is_continuation { FrameType::Continuation(Default::default()) } else { @@ -430,7 +440,7 @@ impl ServerContext { outgoing.headers = HeadersOutgoing::WroteSome(requeued); let frame = Frame::new(frame_type, id); - total_bytes_written += written.len(); + total_bytes_written += write_size; frames.push((frame, PieceList::single(written))); } else { let frame_type = if is_continuation { @@ -479,8 +489,9 @@ impl ServerContext { if fram_size_if_full_piece > max_this_fram { // we can't fit this piece in the current frame, so // we have to split it - frame_len += max_this_fram; - let (written, requeued) = piece.split_at(max_this_fram); + let write_size = max_this_fram - frame_len; + let (written, requeued) = piece.split_at(write_size); + frame_len += write_size; debug!(written_len = %written.len(), requeued_len = %requeued.len(), "splitting piece"); plist.push_back(written); @@ -521,6 +532,7 @@ impl ServerContext { } for (frame, plist) in frames { + debug!(?frame, plist_len = %plist.len(), "writing"); self.write_frame(frame, plist).await?; } @@ -693,7 +705,8 @@ impl ServerContext { outgoing.capacity = next_cap; } else { unreachable!( - "should never write a frame that makes the stream capacity negative" + "should never write a frame that makes the stream capacity negative: outgoing.capacity = {}, payload_len = {}", + outgoing.capacity, payload.len() ) } } From 2c5b0f3f980553e31c05557d3070f3d60fe1ac08 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 17:55:53 +0100 Subject: [PATCH 21/25] 6.9.1 all three cases pass --- crates/fluke/src/h2/server.rs | 167 +++++++++++++++------------------- 1 file changed, 74 insertions(+), 93 deletions(-) diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index 52eea73e..1b6fe478 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -402,10 +402,6 @@ impl ServerContext { debug!(conn_cap = %self.state.outgoing_capacity, strm_cap = %outgoing.capacity, %max_fram, "ready to write"); - let capacity = self.state.outgoing_capacity.min(outgoing.capacity) as usize; - // bytes written this turn, possibly over multiple frames - let mut total_bytes_written = 0; - if outgoing.headers.has_more_to_write() { debug!("writing headers..."); @@ -417,19 +413,16 @@ impl ServerContext { continue 'each_stream; } - 'queue_header_frames: while total_bytes_written < capacity { - debug!(total_bytes_written, capacity, "writing headers..."); + 'queue_header_frames: loop { + debug!("writing headers..."); let is_continuation = matches!(&outgoing.headers, HeadersOutgoing::WroteSome(_)); let piece = outgoing.headers.take_piece(); let piece_len = piece.len(); - let cap_left = capacity - total_bytes_written; - let max_this_fram = max_fram.min(cap_left); - - if piece_len > max_this_fram { - let write_size = max_this_fram; + if piece_len > max_fram { + let write_size = max_fram; let (written, requeued) = piece.split_at(write_size); debug!(%write_size, requeued_len = %requeued.len(), "splitting headers"); let frame_type = if is_continuation { @@ -440,7 +433,6 @@ impl ServerContext { outgoing.headers = HeadersOutgoing::WroteSome(requeued); let frame = Frame::new(frame_type, id); - total_bytes_written += write_size; frames.push((frame, PieceList::single(written))); } else { let frame_type = if is_continuation { @@ -455,7 +447,6 @@ impl ServerContext { }; let frame = Frame::new(frame_type, id); - total_bytes_written += piece_len; frames.push((frame, PieceList::single(piece))); break 'queue_header_frames; @@ -463,6 +454,10 @@ impl ServerContext { } } + let capacity = self.state.outgoing_capacity.min(outgoing.capacity) as usize; + // bytes written this turn, possibly over multiple frames + let mut total_bytes_written = 0; + if outgoing.body.has_more_to_write() { 'queue_body_frames: while total_bytes_written < capacity { // send as much body data as we can, respecting max frame size and @@ -662,94 +657,80 @@ impl ServerContext { mut frame: Frame, payload: PieceList, ) -> Result<(), H2ConnectionError> { - struct FlowControlledFrameInfo { - len: usize, - end_stream: bool, - } - - let flow_controlled_frame_info = match &frame.frame_type { - FrameType::Data(flags) => Some(FlowControlledFrameInfo { - len: payload.len(), - end_stream: flags.contains(DataFlags::EndStream), - }), - FrameType::Headers(flags) => Some(FlowControlledFrameInfo { - len: payload.len(), - end_stream: flags.contains(HeadersFlags::EndStream), - }), - FrameType::Settings(_) => { - // TODO: keep track of whether our new settings have been acknowledged - None - } - _ => { - // muffin. - None - } - }; - - if let Some(flow_controlled_frame_info) = flow_controlled_frame_info { - let mut ss = match self.state.streams.entry(frame.stream_id) { - std::collections::hash_map::Entry::Occupied(entry) => entry, - std::collections::hash_map::Entry::Vacant(_) => unreachable!( - "writing DATA frame for non-existent stream, this should never happen" - ), - }; - - // update stream flow control window - { - let outgoing = match ss.get_mut().outgoing_mut() { - Some(og) => og, - None => unreachable!("writing DATA frame for stream in the wrong state"), + match &frame.frame_type { + FrameType::Data(flags) => { + let mut ss = match self.state.streams.entry(frame.stream_id) { + std::collections::hash_map::Entry::Occupied(entry) => entry, + std::collections::hash_map::Entry::Vacant(_) => unreachable!( + "writing DATA frame for non-existent stream, this should never happen" + ), }; - let payload_len: u32 = payload.len().try_into().unwrap(); - if let Some(next_cap) = outgoing.capacity.checked_sub(payload_len) { - outgoing.capacity = next_cap; - } else { - unreachable!( - "should never write a frame that makes the stream capacity negative: outgoing.capacity = {}, payload_len = {}", - outgoing.capacity, payload.len() - ) - } - } - // now update connection flow control window - { - let payload_len: u32 = payload.len().try_into().unwrap(); - if let Some(next_cap) = self.state.outgoing_capacity.checked_sub(payload_len) { - self.state.outgoing_capacity = next_cap; - } else { - unreachable!( - "should never write a frame that makes the connection capacity negative" - ) + // update stream flow control window + { + let outgoing = match ss.get_mut().outgoing_mut() { + Some(og) => og, + None => { + unreachable!("writing DATA frame for stream in the wrong state") + } + }; + let payload_len: u32 = payload.len().try_into().unwrap(); + if let Some(next_cap) = outgoing.capacity.checked_sub(payload_len) { + outgoing.capacity = next_cap; + } else { + unreachable!( + "should never write a frame that makes the stream capacity negative: outgoing.capacity = {}, payload_len = {}", + outgoing.capacity, payload.len() + ) + } } - } - if flow_controlled_frame_info.end_stream { - // we won't be sending any more data on this stream - self.state - .streams_with_pending_data - .remove(&frame.stream_id); - - match ss.get_mut() { - StreamState::Open { .. } => { - let incoming = match std::mem::take(ss.get_mut()) { - StreamState::Open { incoming, .. } => incoming, - _ => unreachable!(), - }; - // this avoid having to re-insert the stream in the map - *ss.get_mut() = StreamState::HalfClosedLocal { incoming }; + // now update connection flow control window + { + let payload_len: u32 = payload.len().try_into().unwrap(); + if let Some(next_cap) = self.state.outgoing_capacity.checked_sub(payload_len) { + self.state.outgoing_capacity = next_cap; + } else { + unreachable!( + "should never write a frame that makes the connection capacity negative" + ) } - _ => { - // transition to closed - ss.remove(); - debug!( - "Closed stream {} (wrote data w/EndStream), now have {} streams", - frame.stream_id, - self.state.streams.len() - ); + } + + if flags.contains(DataFlags::EndStream) { + // we won't be sending any more data on this stream + self.state + .streams_with_pending_data + .remove(&frame.stream_id); + + match ss.get_mut() { + StreamState::Open { .. } => { + let incoming = match std::mem::take(ss.get_mut()) { + StreamState::Open { incoming, .. } => incoming, + _ => unreachable!(), + }; + // this avoid having to re-insert the stream in the map + *ss.get_mut() = StreamState::HalfClosedLocal { incoming }; + } + _ => { + // transition to closed + ss.remove(); + debug!( + "Closed stream {} (wrote data w/EndStream), now have {} streams", + frame.stream_id, + self.state.streams.len() + ); + } } } } - } + FrameType::Settings(_) => { + // TODO: keep track of whether our new settings have been acknowledged + } + _ => { + // muffin. + } + }; // TODO: enforce max_frame_size from the peer settings, not just u32::max frame.len = payload From 4edfa98edefa87483736031d97add25b5da21ef9 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 18:11:47 +0100 Subject: [PATCH 22/25] fix more tests --- crates/fluke/src/h2/body.rs | 2 +- crates/fluke/src/h2/server.rs | 78 ++++++++++++++++++++++------------- crates/fluke/src/h2/types.rs | 24 ++++++++--- 3 files changed, 69 insertions(+), 35 deletions(-) diff --git a/crates/fluke/src/h2/body.rs b/crates/fluke/src/h2/body.rs index c749cfb0..dfed995c 100644 --- a/crates/fluke/src/h2/body.rs +++ b/crates/fluke/src/h2/body.rs @@ -14,7 +14,7 @@ pub(crate) struct StreamIncoming { // incoming capacity (that we decide, we get to tell // the peer how much we can handle with window updates) - pub(crate) capacity: u32, + pub(crate) capacity: i64, } // FIXME: don't use eyre, do proper error handling diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index 1b6fe478..d4b41cbf 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -39,6 +39,8 @@ use crate::{ ExpectResponseHeaders, Headers, Method, Request, Responder, ServerDriver, }; +pub const MAX_WINDOW_SIZE: i64 = u32::MAX as i64; + /// HTTP/2 server configuration pub struct ServerConf { pub max_streams: u32, @@ -388,7 +390,7 @@ impl ServerContext { .collect(); 'each_stream: for id in streams_with_pending_data { - if self.state.outgoing_capacity == 0 { + if self.state.outgoing_capacity <= 0 { // that's all we can do break 'each_stream; } @@ -675,26 +677,29 @@ impl ServerContext { } }; let payload_len: u32 = payload.len().try_into().unwrap(); - if let Some(next_cap) = outgoing.capacity.checked_sub(payload_len) { - outgoing.capacity = next_cap; - } else { + let next_cap = outgoing.capacity - payload_len as i64; + + if next_cap < 0 { unreachable!( - "should never write a frame that makes the stream capacity negative: outgoing.capacity = {}, payload_len = {}", - outgoing.capacity, payload.len() - ) + "should never write a frame that makes the stream capacity negative: outgoing.capacity = {}, payload_len = {}", + outgoing.capacity, payload.len() + ) } + outgoing.capacity = next_cap; } // now update connection flow control window { let payload_len: u32 = payload.len().try_into().unwrap(); - if let Some(next_cap) = self.state.outgoing_capacity.checked_sub(payload_len) { - self.state.outgoing_capacity = next_cap; - } else { + let next_cap = self.state.outgoing_capacity - payload_len as i64; + + if next_cap < 0 { unreachable!( - "should never write a frame that makes the connection capacity negative" - ) + "should never write a frame that makes the connection capacity negative: outgoing_capacity = {}, payload_len = {}", + self.state.outgoing_capacity, payload.len() + ) } + self.state.outgoing_capacity = next_cap; } if flags.contains(DataFlags::EndStream) { @@ -1013,6 +1018,27 @@ impl ServerContext { .set_max_table_size(settings.header_table_size as usize); debug!("Peer sent us {settings:#?}"); + let initial_window_size_delta = (settings.initial_window_size as i64) + - (self.state.peer_settings.initial_window_size as i64); + // apply that delta to the connection and all streams + self.state.outgoing_capacity += initial_window_size_delta; + if self.state.outgoing_capacity > MAX_WINDOW_SIZE { + return Err(H2ConnectionError::ConnectionWindowSizeOverflowDueToSettings); + } + + for (id, stream) in self.state.streams.iter_mut() { + if let Some(outgoing) = stream.outgoing_mut() { + outgoing.capacity += initial_window_size_delta; + if outgoing.capacity > MAX_WINDOW_SIZE { + return Err( + H2ConnectionError::StreamWindowSizeOverflowDueToSettings { + stream_id: *id, + }, + ); + } + } + } + self.state.peer_settings = settings; let frame = Frame::new( @@ -1078,12 +1104,11 @@ impl ServerContext { } if frame.stream_id == StreamId::CONNECTION { - let new_capacity = match self.state.outgoing_capacity.checked_add(increment) { - Some(x) => x, - None => { - return Err(H2ConnectionError::WindowUpdateOverflow); - } + let new_capacity = self.state.outgoing_capacity + increment as i64; + if new_capacity > MAX_WINDOW_SIZE { + return Err(H2ConnectionError::WindowUpdateOverflow); }; + debug!(old_capacity = %self.state.outgoing_capacity, %new_capacity, "connection window update"); self.state.outgoing_capacity = new_capacity; self.state.send_data_maybe.notify_one(); @@ -1102,15 +1127,13 @@ impl ServerContext { } }; - let new_capacity = match outgoing.capacity.checked_add(increment) { - Some(x) => x, - None => { - // reset the stream - self.rst(frame.stream_id, H2StreamError::WindowUpdateOverflow) - .await?; - return Ok(()); - } - }; + let new_capacity = outgoing.capacity + increment as i64; + if new_capacity > MAX_WINDOW_SIZE { + // reset the stream + self.rst(frame.stream_id, H2StreamError::WindowUpdateOverflow) + .await?; + return Ok(()); + } debug!(old_capacity = %outgoing.capacity, %new_capacity, "stream window update"); outgoing.capacity = new_capacity; @@ -1388,8 +1411,7 @@ impl ServerContext { }; let incoming = StreamIncoming { - // FIXME: handle negative window size, cf. https://httpwg.org/specs/rfc9113.html#InitialWindowSize - capacity: self.state.self_settings.initial_window_size, + capacity: self.state.self_settings.initial_window_size as _, tx: piece_tx, }; let outgoing: StreamOutgoing = self.state.mk_stream_outgoing(); diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index fb211556..563da0c5 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -30,8 +30,8 @@ pub(crate) struct ConnState { pub(crate) send_data_maybe: Notify, pub(crate) streams_with_pending_data: HashSet, - pub(crate) incoming_capacity: u32, - pub(crate) outgoing_capacity: u32, + pub(crate) incoming_capacity: i64, + pub(crate) outgoing_capacity: i64, } impl Default for ConnState { @@ -49,8 +49,8 @@ impl Default for ConnState { incoming_capacity: 0, outgoing_capacity: 0, }; - s.incoming_capacity = s.self_settings.initial_window_size; - s.outgoing_capacity = s.peer_settings.initial_window_size; + s.incoming_capacity = s.self_settings.initial_window_size as _; + s.outgoing_capacity = s.peer_settings.initial_window_size as _; s } @@ -62,7 +62,7 @@ impl ConnState { StreamOutgoing { headers: HeadersOutgoing::WaitingForHeaders, body: BodyOutgoing::StillReceiving(Default::default()), - capacity: self.peer_settings.initial_window_size, + capacity: self.peer_settings.initial_window_size as _, } } } @@ -177,7 +177,7 @@ pub(crate) struct StreamOutgoing { // window size of the stream, ie. how many bytes // we can send to the receiver before waiting. - pub(crate) capacity: u32, + pub(crate) capacity: i64, } #[derive(Default)] @@ -411,6 +411,12 @@ pub(crate) enum H2ConnectionError { #[error("received window update that made the window size overflow")] WindowUpdateOverflow, + #[error("received initial window size settings update that made the connection window size overflow")] + ConnectionWindowSizeOverflowDueToSettings, + + #[error("received initial window size settings update that made the connection window size overflow")] + StreamWindowSizeOverflowDueToSettings { stream_id: StreamId }, + #[error("received window update frame with invalid length {len}")] WindowUpdateInvalidLength { len: usize }, } @@ -427,6 +433,12 @@ impl H2ConnectionError { H2ConnectionError::WindowUpdateInvalidLength { .. } => KnownErrorCode::FrameSizeError, // flow control errors H2ConnectionError::WindowUpdateOverflow => KnownErrorCode::FlowControlError, + H2ConnectionError::ConnectionWindowSizeOverflowDueToSettings => { + KnownErrorCode::FlowControlError + } + H2ConnectionError::StreamWindowSizeOverflowDueToSettings { .. } => { + KnownErrorCode::FlowControlError + } // compression errors H2ConnectionError::CompressionError(_) => KnownErrorCode::CompressionError, // stream closed error From c7421167bcaa7ea793935f3a7a9ba1fc4eaa7a3c Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 18:13:45 +0100 Subject: [PATCH 23/25] remove unused methods --- crates/fluke/src/h2/types.rs | 38 ------------------------------------ 1 file changed, 38 deletions(-) diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index 563da0c5..71b399d8 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -134,33 +134,6 @@ pub(crate) enum StreamState { } impl StreamState { - /// Get the inner `StreamIncoming` if the state is `Open` or `HalfClosedLocal`. - pub(crate) fn incoming_ref(&self) -> Option<&StreamIncoming> { - match self { - StreamState::Open { incoming, .. } => Some(incoming), - StreamState::HalfClosedLocal { incoming, .. } => Some(incoming), - _ => None, - } - } - - /// Get the inner `StreamIncoming` if the state is `Open` or `HalfClosedLocal`. - pub(crate) fn incoming_mut(&mut self) -> Option<&mut StreamIncoming> { - match self { - StreamState::Open { incoming, .. } => Some(incoming), - StreamState::HalfClosedLocal { incoming, .. } => Some(incoming), - _ => None, - } - } - - /// Get the inner `StreamOutgoing` if the state is `Open` or `HalfClosedRemote`. - pub(crate) fn outgoing_ref(&self) -> Option<&StreamOutgoing> { - match self { - StreamState::Open { outgoing, .. } => Some(outgoing), - StreamState::HalfClosedRemote { outgoing, .. } => Some(outgoing), - _ => None, - } - } - /// Get the inner `StreamOutgoing` if the state is `Open` or `HalfClosedRemote`. pub(crate) fn outgoing_mut(&mut self) -> Option<&mut StreamOutgoing> { match self { @@ -197,17 +170,6 @@ pub(crate) enum HeadersOutgoing { } impl HeadersOutgoing { - /// It's still possible that we might receive more headers from the user. - #[inline(always)] - pub(crate) fn might_receive_more(&self) -> bool { - match self { - HeadersOutgoing::WaitingForHeaders => true, - HeadersOutgoing::WroteNone(_) => true, - HeadersOutgoing::WroteSome(_) => true, - HeadersOutgoing::WroteAll => false, - } - } - #[inline(always)] pub(crate) fn has_more_to_write(&self) -> bool { match self { From 858a39c4064c32222cccfa8b1aef940c533c8884 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Fri, 29 Mar 2024 18:25:56 +0100 Subject: [PATCH 24/25] Implement final flow control touches --- crates/fluke/src/h2/server.rs | 47 +++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index d4b41cbf..1da37086 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -1017,25 +1017,30 @@ impl ServerContext { self.hpack_enc .set_max_table_size(settings.header_table_size as usize); - debug!("Peer sent us {settings:#?}"); let initial_window_size_delta = (settings.initial_window_size as i64) - (self.state.peer_settings.initial_window_size as i64); - // apply that delta to the connection and all streams - self.state.outgoing_capacity += initial_window_size_delta; - if self.state.outgoing_capacity > MAX_WINDOW_SIZE { - return Err(H2ConnectionError::ConnectionWindowSizeOverflowDueToSettings); - } + debug!(%initial_window_size_delta, "Peer sent us {settings:#?}"); + + let mut maybe_send_data = false; + // apply that delta to all streams for (id, stream) in self.state.streams.iter_mut() { if let Some(outgoing) = stream.outgoing_mut() { - outgoing.capacity += initial_window_size_delta; - if outgoing.capacity > MAX_WINDOW_SIZE { + let next_cap = outgoing.capacity + initial_window_size_delta; + if next_cap > MAX_WINDOW_SIZE { return Err( H2ConnectionError::StreamWindowSizeOverflowDueToSettings { stream_id: *id, }, ); } + // if capacity was negative or zero, and is now greater than zero, + // we need to maybe send data + if next_cap > 0 && outgoing.capacity <= 0 { + debug!(?id, %next_cap, "stream capacity was <= 0, now > 0"); + maybe_send_data = true; + } + outgoing.capacity = next_cap; } } @@ -1047,6 +1052,10 @@ impl ServerContext { ); self.write_frame(frame, PieceList::default()).await?; debug!("Acknowledged peer settings"); + + if maybe_send_data { + self.state.send_data_maybe.notify_one(); + } } } FrameType::PushPromise => { @@ -1135,15 +1144,21 @@ impl ServerContext { return Ok(()); } - debug!(old_capacity = %outgoing.capacity, %new_capacity, "stream window update"); + let old_capacity = outgoing.capacity; + debug!(stream_id = %frame.stream_id, %old_capacity, %new_capacity, "stream window update"); outgoing.capacity = new_capacity; - if self.state.outgoing_capacity > 0 - && self - .state - .streams_with_pending_data - .contains(&frame.stream_id) - { - self.state.send_data_maybe.notify_one(); + + // insert into streams_with_pending_data if the old capacity was <= zero + // and the new capacity is > zero + if old_capacity <= 0 && new_capacity > 0 { + debug!(conn_capacity = %self.state.outgoing_capacity, "stream capacity is newly positive, inserting in streams_with_pending_data"); + self.state.streams_with_pending_data.insert(frame.stream_id); + + // if the connection has capacity, notify! + if self.state.outgoing_capacity > 0 { + debug!(stream_id = ?frame.stream_id, "stream window update, maybe send data"); + self.state.send_data_maybe.notify_one(); + } } } } From 86feb9d4be2be0f1a02250264cb0edb320712dc1 Mon Sep 17 00:00:00 2001 From: Amos Wenger Date: Sat, 30 Mar 2024 15:53:30 +0100 Subject: [PATCH 25/25] Fix linux build --- crates/fluke-buffet/src/piece.rs | 2 +- crates/fluke-maybe-uring/src/net/net_uring.rs | 12 ++++++------ crates/fluke/src/h2/server.rs | 4 ++-- crates/fluke/src/h2/types.rs | 6 ------ test-crates/fluke-tls-sample/Cargo.lock | 14 +++++++------- 5 files changed, 16 insertions(+), 22 deletions(-) diff --git a/crates/fluke-buffet/src/piece.rs b/crates/fluke-buffet/src/piece.rs index 7e92f2ab..9cba1392 100644 --- a/crates/fluke-buffet/src/piece.rs +++ b/crates/fluke-buffet/src/piece.rs @@ -349,7 +349,7 @@ impl Deref for PieceStr { type Target = str; fn deref(&self) -> &Self::Target { - unsafe { std::str::from_utf8_unchecked(&self.piece.as_ref()) } + unsafe { std::str::from_utf8_unchecked(&self.piece) } } } diff --git a/crates/fluke-maybe-uring/src/net/net_uring.rs b/crates/fluke-maybe-uring/src/net/net_uring.rs index a019b47c..9803535b 100644 --- a/crates/fluke-maybe-uring/src/net/net_uring.rs +++ b/crates/fluke-maybe-uring/src/net/net_uring.rs @@ -1,6 +1,5 @@ use std::{ - net::{Shutdown, SocketAddr}, - rc::Rc, + collections::VecDeque, net::{Shutdown, SocketAddr}, rc::Rc }; use crate::{ @@ -51,12 +50,13 @@ impl WriteOwned for TcpWriteHalf { (res, b.0) } - async fn writev(&mut self, list: Vec) -> BufResult> { - use bytemuck::allocation::TransparentWrapperAlloc; + async fn writev(&mut self, list: VecDeque) -> BufResult> { + // TODO: use bytemuck::allcation::TransparentWrapperAlloc again, + // with wrap_vec and peel_vec - let list = BufCompat::wrap_vec(list); + let list: Vec> = list.into_iter().map(BufCompat).collect(); let (res, list) = self.0.writev(list).await; - let list: Vec = BufCompat::peel_vec(list); + let list: VecDeque = list.into_iter().map(|b| b.0).collect(); (res, list) } diff --git a/crates/fluke/src/h2/server.rs b/crates/fluke/src/h2/server.rs index 1da37086..ac64f5b4 100644 --- a/crates/fluke/src/h2/server.rs +++ b/crates/fluke/src/h2/server.rs @@ -516,7 +516,7 @@ impl ServerContext { flags |= DataFlags::EndStream; } - let frame = Frame::new(FrameType::Data(flags.into()), id); + let frame = Frame::new(FrameType::Data(flags), id); debug!(?frame, %frame_len, "queuing"); frames.push((frame, plist)); total_bytes_written += frame_len; @@ -634,7 +634,7 @@ impl ServerContext { match &mut outgoing.body { BodyOutgoing::StillReceiving(pieces) => { let pieces = std::mem::take(pieces); - if pieces.len() > 0 { + if pieces.is_empty() { // we'll need to send a zero-length data frame self.state.send_data_maybe.notify_one(); } diff --git a/crates/fluke/src/h2/types.rs b/crates/fluke/src/h2/types.rs index 71b399d8..223bdcbb 100644 --- a/crates/fluke/src/h2/types.rs +++ b/crates/fluke/src/h2/types.rs @@ -373,9 +373,6 @@ pub(crate) enum H2ConnectionError { #[error("received window update that made the window size overflow")] WindowUpdateOverflow, - #[error("received initial window size settings update that made the connection window size overflow")] - ConnectionWindowSizeOverflowDueToSettings, - #[error("received initial window size settings update that made the connection window size overflow")] StreamWindowSizeOverflowDueToSettings { stream_id: StreamId }, @@ -395,9 +392,6 @@ impl H2ConnectionError { H2ConnectionError::WindowUpdateInvalidLength { .. } => KnownErrorCode::FrameSizeError, // flow control errors H2ConnectionError::WindowUpdateOverflow => KnownErrorCode::FlowControlError, - H2ConnectionError::ConnectionWindowSizeOverflowDueToSettings => { - KnownErrorCode::FlowControlError - } H2ConnectionError::StreamWindowSizeOverflowDueToSettings { .. } => { KnownErrorCode::FlowControlError } diff --git a/test-crates/fluke-tls-sample/Cargo.lock b/test-crates/fluke-tls-sample/Cargo.lock index 56a42b3b..cce20d78 100644 --- a/test-crates/fluke-tls-sample/Cargo.lock +++ b/test-crates/fluke-tls-sample/Cargo.lock @@ -122,9 +122,9 @@ checksum = "7ff69b9dd49fd426c69a0db9fc04dd934cdb6645ff000864d98f7e2af8830eaa" [[package]] name = "bytemuck" -version = "1.14.3" +version = "1.15.0" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a2ef034f05691a48569bd920a96c81b9d91bbad1ab5ac7c4616c1f6ef36cb79f" +checksum = "5d6d68c57235a3a081186990eca2867354726650f42f7516ca50c28d6281fd15" [[package]] name = "byteorder" @@ -623,7 +623,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0c2a198fb6b0eada2a8df47933734e6d35d350665a33a3593d7164fa52c75c19" dependencies = [ "cfg-if", - "windows-targets 0.48.5", + "windows-targets 0.52.4", ] [[package]] @@ -1188,18 +1188,18 @@ dependencies = [ [[package]] name = "thiserror" -version = "1.0.57" +version = "1.0.58" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "1e45bcbe8ed29775f228095caf2cd67af7a4ccf756ebff23a306bf3e8b47b24b" +checksum = "03468839009160513471e86a034bb2c5c0e4baae3b43f79ffc55c4a5427b3297" dependencies = [ "thiserror-impl", ] [[package]] name = "thiserror-impl" -version = "1.0.57" +version = "1.0.58" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a953cb265bef375dae3de6663da4d3804eee9682ea80d8e2542529b73c531c81" +checksum = "c61f3ba182994efc43764a46c018c347bc492c79f024e705f46567b418f6d4f7" dependencies = [ "proc-macro2", "quote",