From 7fcba8b61adcbaab97f4dbe8a8f816390fda5a96 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 3 May 2024 17:49:07 +0200 Subject: [PATCH 1/6] fix(http3): always qlog on send_buffer() `neqo_http3::SendMessage` calls `qlog::h3_data_moved_down()` whenever it moves data down to the QUIC layer. `SendMessage` moves data down to the QUIC layer either directly via `self.stream.send_atomic` or indirectly buffered through `self.stream.send_buffer`. Previously only one of the 3 calls to `self.stream.send_buffer` would thereafter call `qlog::h3_data_moved_down()`. This commit moves the `h3_data_moved_down` call into `self.stream.send_buffer`, thus ensuring the function is always called when data is moved. In addition, `self.stream.send_atomic` now as well does the qlog call, thus containing all qlog logic in `buffered_send_stream.rs` instead of `send_message.rs`. --- neqo-http3/src/buffered_send_stream.rs | 7 ++++++- neqo-http3/src/send_message.rs | 4 +--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/neqo-http3/src/buffered_send_stream.rs b/neqo-http3/src/buffered_send_stream.rs index 4f6761fa80..c82158f55b 100644 --- a/neqo-http3/src/buffered_send_stream.rs +++ b/neqo-http3/src/buffered_send_stream.rs @@ -7,7 +7,7 @@ use neqo_common::qtrace; use neqo_transport::{Connection, StreamId}; -use crate::Res; +use crate::{qlog, Res}; #[derive(Debug, PartialEq, Eq)] pub enum BufferedStream { @@ -75,6 +75,8 @@ impl BufferedStream { *buf = b; } } + + qlog::h3_data_moved_down(conn.qlog_mut(), *stream_id, sent); } Ok(sent) } @@ -88,6 +90,9 @@ impl BufferedStream { if let Self::Initialized { stream_id, buf } = self { if buf.is_empty() { let res = conn.stream_send_atomic(*stream_id, to_send)?; + if res { + qlog::h3_data_moved_down(conn.qlog_mut(), *stream_id, to_send.len()); + } Ok(res) } else { Ok(false) diff --git a/neqo-http3/src/send_message.rs b/neqo-http3/src/send_message.rs index 15965c44f6..7fb37beb70 100644 --- a/neqo-http3/src/send_message.rs +++ b/neqo-http3/src/send_message.rs @@ -13,7 +13,7 @@ use neqo_transport::{Connection, StreamId}; use crate::{ frames::HFrame, headers_checks::{headers_valid, is_interim, trailers_valid}, - qlog, BufferedStream, CloseType, Error, Http3StreamInfo, Http3StreamType, HttpSendStream, Res, + BufferedStream, CloseType, Error, Http3StreamInfo, Http3StreamType, HttpSendStream, Res, SendStream, SendStreamEvents, Stream, }; @@ -216,7 +216,6 @@ impl SendStream for SendMessage { .send_atomic(conn, &buf[..to_send]) .map_err(|e| Error::map_stream_send_errors(&e))?; debug_assert!(sent); - qlog::h3_data_moved_down(conn.qlog_mut(), self.stream_id(), to_send); Ok(to_send) } @@ -243,7 +242,6 @@ impl SendStream for SendMessage { /// info that the stream has been closed.) fn send(&mut self, conn: &mut Connection) -> Res<()> { let sent = Error::map_error(self.stream.send_buffer(conn), Error::HttpInternal(5))?; - qlog::h3_data_moved_down(conn.qlog_mut(), self.stream_id(), sent); qtrace!([self], "{} bytes sent", sent); if !self.stream.has_buffered_data() { From 001666a12c04b65a0455c2b1c1ec253b4e9b7c71 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Fri, 3 May 2024 19:55:43 +0200 Subject: [PATCH 2/6] Trigger benchmark run From 29ddca58df11ea2f8e70b583d8b9a43f5dcecce1 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 4 May 2024 08:27:30 +0200 Subject: [PATCH 3/6] Don't qlog if buffer is empty --- neqo-http3/src/buffered_send_stream.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/neqo-http3/src/buffered_send_stream.rs b/neqo-http3/src/buffered_send_stream.rs index c82158f55b..35ee86866f 100644 --- a/neqo-http3/src/buffered_send_stream.rs +++ b/neqo-http3/src/buffered_send_stream.rs @@ -74,9 +74,8 @@ impl BufferedStream { let b = buf.split_off(sent); *buf = b; } + qlog::h3_data_moved_down(conn.qlog_mut(), *stream_id, sent); } - - qlog::h3_data_moved_down(conn.qlog_mut(), *stream_id, sent); } Ok(sent) } From d96372c38940dac4cf2bbb788ab8d6e78fbb2351 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 4 May 2024 08:35:18 +0200 Subject: [PATCH 4/6] Fix typo --- neqo-http3/src/buffered_send_stream.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/neqo-http3/src/buffered_send_stream.rs b/neqo-http3/src/buffered_send_stream.rs index 35ee86866f..fcf4aa05ae 100644 --- a/neqo-http3/src/buffered_send_stream.rs +++ b/neqo-http3/src/buffered_send_stream.rs @@ -38,7 +38,7 @@ impl BufferedStream { /// # Panics /// - /// If the `BufferedStream` is initialized more than one it will panic. + /// If the `BufferedStream` is initialized more than once, it will panic. pub fn init(&mut self, stream_id: StreamId) { debug_assert!(&Self::Uninitialized == self); *self = Self::Initialized { From d15e3e0c6ad8b54c0ad1445e35da0a9cbe2e4265 Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 4 May 2024 08:35:52 +0200 Subject: [PATCH 5/6] Use early return to keep indentation short --- neqo-http3/src/buffered_send_stream.rs | 49 +++++++++++++------------- 1 file changed, 24 insertions(+), 25 deletions(-) diff --git a/neqo-http3/src/buffered_send_stream.rs b/neqo-http3/src/buffered_send_stream.rs index fcf4aa05ae..92f4839cd0 100644 --- a/neqo-http3/src/buffered_send_stream.rs +++ b/neqo-http3/src/buffered_send_stream.rs @@ -63,20 +63,21 @@ impl BufferedStream { /// Returns `neqo_transport` errors. pub fn send_buffer(&mut self, conn: &mut Connection) -> Res { let label = ::neqo_common::log_subject!(::log::Level::Debug, self); - let mut sent = 0; - if let Self::Initialized { stream_id, buf } = self { - if !buf.is_empty() { - qtrace!([label], "sending data."); - sent = conn.stream_send(*stream_id, &buf[..])?; - if sent == buf.len() { - buf.clear(); - } else { - let b = buf.split_off(sent); - *buf = b; - } - qlog::h3_data_moved_down(conn.qlog_mut(), *stream_id, sent); - } + let Self::Initialized { stream_id, buf } = self else { + return Ok(0); + }; + if buf.is_empty() { + return Ok(0); + } + qtrace!([label], "sending data."); + let sent = conn.stream_send(*stream_id, &buf[..])?; + if sent == buf.len() { + buf.clear(); + } else { + let b = buf.split_off(sent); + *buf = b; } + qlog::h3_data_moved_down(conn.qlog_mut(), *stream_id, sent); Ok(sent) } @@ -86,19 +87,17 @@ impl BufferedStream { pub fn send_atomic(&mut self, conn: &mut Connection, to_send: &[u8]) -> Res { // First try to send anything that is in the buffer. self.send_buffer(conn)?; - if let Self::Initialized { stream_id, buf } = self { - if buf.is_empty() { - let res = conn.stream_send_atomic(*stream_id, to_send)?; - if res { - qlog::h3_data_moved_down(conn.qlog_mut(), *stream_id, to_send.len()); - } - Ok(res) - } else { - Ok(false) - } - } else { - Ok(false) + let Self::Initialized { stream_id, buf } = self else { + return Ok(false); + }; + if !buf.is_empty() { + return Ok(false); + } + let res = conn.stream_send_atomic(*stream_id, to_send)?; + if res { + qlog::h3_data_moved_down(conn.qlog_mut(), *stream_id, to_send.len()); } + Ok(res) } #[must_use] From ba11ce1c793bdf132b5529978b81b3cf31d714ea Mon Sep 17 00:00:00 2001 From: Max Inden Date: Sat, 4 May 2024 08:44:10 +0200 Subject: [PATCH 6/6] Don't qlog if sent is 0 --- neqo-http3/src/buffered_send_stream.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/neqo-http3/src/buffered_send_stream.rs b/neqo-http3/src/buffered_send_stream.rs index 92f4839cd0..60da0512b5 100644 --- a/neqo-http3/src/buffered_send_stream.rs +++ b/neqo-http3/src/buffered_send_stream.rs @@ -71,7 +71,9 @@ impl BufferedStream { } qtrace!([label], "sending data."); let sent = conn.stream_send(*stream_id, &buf[..])?; - if sent == buf.len() { + if sent == 0 { + return Ok(0); + } else if sent == buf.len() { buf.clear(); } else { let b = buf.split_off(sent);