Skip to content

Conversation

@gretchenfrage
Copy link
Collaborator

This draft PR currently targets a temporary base branch formed by merging #2230 and #2226 into main.


Overall, this PR makes the refactored write_source API proposed in #2230 public and threads it through to a public API in Quinn as well. This has value both for simplicity and utility. From the simplicity side, it allows us to supersede and delete the quinn::SendStream::execute_poll function.

From the utility side, this API, for example, should be considered to close #2229, as it could be used to implement a write_atomic method purely on top of existing public methods as such:

diff --git a/quinn/src/send_stream.rs b/quinn/src/send_stream.rs
index fcea07cf..b5a4240c 100644
--- a/quinn/src/send_stream.rs
+++ b/quinn/src/send_stream.rs
@@ -1,4 +1,5 @@
 use std::{
+    convert::identity,
     fmt,
     future::{Future, poll_fn},
     io,
@@ -101,6 +102,20 @@ impl SendStream {
         Ok(())
     }
 
+    pub async fn write_atomic(&mut self, buf: &[u8]) -> Result<(), WriteAtomicError> {
+        self.write_source(|limit, chunks| {
+            if limit >= buf.len() {
+                chunks.push(buf.to_vec().into());
+                Ok(())
+            } else {
+                Err(WriteAtomicError::TooLarge)
+            }
+        })
+        .await
+        .map_err(|e| WriteAtomicError::WriteError(e.error))
+        .and_then(identity)
+    }
+
     /// Attempts to write bytes into this stream from a byte-providing callback
     ///
     /// This is a low-level writing API that can be used to perform writes in a way that is atomic
@@ -465,3 +480,14 @@ impl<F> From<WriteSourceError<F>> for io::Error {
         e.error.into()
     }
 }
+
+/// Errors that arise from writing to a stream atomically
+#[derive(Debug, Error, Clone, PartialEq, Eq)]
+pub enum WriteAtomicError {
+    /// The write was too large to send atomically at this time
+    #[error("too large for atomic write")]
+    TooLarge,
+    /// Some other write error occurred
+    #[error("stream write error")]
+    WriteError(WriteError),
+}

The write_source method now returns ownership of the source callback if
it errors.
This commit makes there be fewer places where SendStream::execute_poll
is called directly, in preparation for a refactor of execute_poll.
AsyncWrite implementations now go through poll_write, and poll_write now
goes through the write future. This means that execute_poll is now only
called directly from write and write_chunks.
This commit adds a public API to SendStream which exposes proto's
new write_source functionality in an async way. Its implementation
is based on the code in execute_poll.
It's a bit nicer than just a (WriteError, F) tuple.
The SendStream::execute_poll private method is removed entirely. write
and write_chunks instead call the async version of write_source, using
the same stage_buf / stage_chunks helper functions that proto uses in
its non-async equivalents of these methods.
@gretchenfrage
Copy link
Collaborator Author

Closing in favor of #2260 or successor.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant