atomicWrite support#3475
Conversation
jesup
requested review from
KershawChang,
larseggert,
martinthomson and
mxinden
as code owners
There was a problem hiding this comment.
Pull request overview
This PR adds WebTransport “atomic write” support and exposes send-stream flow-control/buffering introspection by plumbing transport send-buffer state up through the HTTP/3 client layer.
Changes:
- Add transport send-stream
buffered()reporting and expose it viaConnection::stream_send_buffered(). - Extend the HTTP/3 send-stream abstraction to allow downcasting, enabling a WebTransport-specific atomic send path.
- Add WebTransport tests covering atomic sends and flow-control/buffered-byte reporting.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| neqo-transport/src/send_stream.rs | Adds SendStream::buffered() to report tx-buffered bytes. |
| neqo-transport/src/connection/mod.rs | Exposes stream_send_buffered() on Connection. |
| neqo-http3/src/lib.rs | Extends internal SendStream trait with as_any_mut() for downcasting. |
| neqo-http3/src/send_message.rs | Implements as_any_mut() for SendMessage. |
| neqo-http3/src/features/extended_connect/session.rs | Implements as_any_mut() for session send-stream wrapper. |
| neqo-http3/src/features/extended_connect/webtransport_streams.rs | Adds WebTransport-specific send_atomic() and as_any_mut() implementation. |
| neqo-http3/src/connection.rs | Adds webtransport_send_stream_atomic() that downcasts to WebTransportSendStream. |
| neqo-http3/src/connection_client.rs | Adds public client APIs for flow-control info + atomic send. |
| neqo-http3/src/features/extended_connect/tests/webtransport/mod.rs | Registers new atomic-write test module. |
| neqo-http3/src/features/extended_connect/tests/webtransport/atomic_write.rs | Adds test coverage for atomic writes and flow-control/buffering info. |
jesup
force-pushed
the
users/jesup/waitUntilAvailable
branch
from
6ffa7e2 to
c4a9fe7
Compare
jesup
force-pushed
the
users/jesup/atomicWrite
branch
from
ce6b89f to
448e75a
Compare
|
This PR is part of a stack of 13 bookmarks:
Created with jj-stack |
Failed Interop TestsNone ❓ All resultsSucceeded Interop TestsQUIC Interop Runner, client vs. server neqo-pr as clientNone ❓ neqo-pr as serverNone ❓ Unsupported Interop TestsQUIC Interop Runner, client vs. server neqo-pr as clientNone ❓ neqo-pr as serverNone ❓ |
martinthomson
left a comment
martinthomson
left a comment
There was a problem hiding this comment.
I believe that you have uncovered a bug to fix that is a bit deeper than this superficial fix.
| self.stream.buffer(buf); | ||
| _ = self.stream.send_buffer(conn, now)?; | ||
| Ok(()) | ||
| Ok(true) |
| Ok(conn.stream_send_atomic(self.stream_id, data)?) | ||
| } else { | ||
| // Can't send atomically yet, init buffer not fully sent | ||
| Ok(false) |
There was a problem hiding this comment.
So this is the only false return case? What about when the stream hasn't enough space to send?
| }; | ||
| self.stream.encode_with(|e| data_frame.encode(e)); | ||
| self.stream.buffer(buf); | ||
| _ = self.stream.send_buffer(conn, now)?; |
There was a problem hiding this comment.
IMPORTANT: I don't think that this function is truly atomic. The stream has a method called send_atomic, but this goes nowhere near that. So I think we're into false promises here. I DON'T think that it's enough to replace this line and the next with a simple call. What you will need to do is ensure that the HTTP/3-layer buffer is passed down AND that a new buffer (including the length field) can be passed down atomically.
No description provided.