Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove panic-on-drop in quinn_proto::Chunks #1983

Merged
merged 2 commits into from
Sep 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion quinn-proto/src/connection/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ pub use streams::StreamsState;
use streams::StreamsState;
pub use streams::{
BytesSource, Chunks, ClosedStream, FinishError, ReadError, ReadableError, RecvStream,
SendStream, StreamEvent, Streams, WriteError, Written,
SendStream, ShouldTransmit, StreamEvent, Streams, WriteError, Written,
};

mod timer;
Expand Down
32 changes: 23 additions & 9 deletions quinn-proto/src/connection/streams/recv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,17 @@ impl Recv {
}
}

/// Chunks
/// Chunks returned from [`RecvStream::read()`][crate::RecvStream::read].
///
/// ### Note: Finalization Needed
/// Bytes read from the stream are not released from the congestion window until
/// either [`Self::finalize()`] is called, or this type is dropped.
///
/// It is recommended that you call [`Self::finalize()`] because it returns a flag
/// telling you whether reading from the stream has resulted in the need to transmit a packet.
///
/// If this type is leaked, the stream will remain blocked on the remote peer until
/// another read from the stream is done.
pub struct Chunks<'a> {
id: StreamId,
ordered: bool,
Expand Down Expand Up @@ -302,17 +312,21 @@ impl<'a> Chunks<'a> {
}
}

/// Finalize
/// Mark the read data as consumed from the stream.
///
/// The number of read bytes will be released from the congestion window,
/// allowing the remote peer to send more data if it was previously blocked.
///
/// If [`ShouldTransmit::should_transmit()`] returns `true`,
/// a packet needs to be sent to the peer informing them that the stream is unblocked.
/// This means that you should call [`Connection::poll_transmit()`][crate::Connection::poll_transmit]
/// and send the returned packet as soon as is reasonable, to unblock the remote peer.
pub fn finalize(mut self) -> ShouldTransmit {
self.finalize_inner(false)
self.finalize_inner()
}

fn finalize_inner(&mut self, drop: bool) -> ShouldTransmit {
fn finalize_inner(&mut self) -> ShouldTransmit {
let state = mem::replace(&mut self.state, ChunksState::Finalized);
debug_assert!(
!drop || matches!(state, ChunksState::Finalized),
"finalize must be called before drop"
);
if let ChunksState::Finalized = state {
// Noop on repeated calls
return ShouldTransmit(false);
Expand Down Expand Up @@ -344,7 +358,7 @@ impl<'a> Chunks<'a> {

impl<'a> Drop for Chunks<'a> {
fn drop(&mut self) {
let _ = self.finalize_inner(true);
let _ = self.finalize_inner();
}
}

Expand Down
4 changes: 2 additions & 2 deletions quinn-proto/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ mod connection;
pub use crate::connection::{
BytesSource, Chunk, Chunks, ClosedStream, Connection, ConnectionError, ConnectionStats,
Datagrams, Event, FinishError, FrameStats, PathStats, ReadError, ReadableError, RecvStream,
RttEstimator, SendDatagramError, SendStream, StreamEvent, Streams, UdpStats, WriteError,
Written,
RttEstimator, SendDatagramError, SendStream, ShouldTransmit, StreamEvent, Streams, UdpStats,
WriteError, Written,
};

mod config;
Expand Down