Make PacketBuilder own the write buffer #2195
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This TransmitBuf is a wrapper around the buffer in which datagrams are being created. It keeps track of the state required to know the boundaries of the datagrams in the buffer.
This removes the extra arguments from Packetbuilder::new that tell the builder about datagram boundaries in favour of using the state of TransmitBuf directly.
This moves the logic of updating the buffer for new datagrams into a function on the TransmitBuf. This centralises all the manipulation of these variables into one logical place, ensuring all the invariants are upheld.
This helps encapsulation by ensuring that no outside users can mutate these fields. Reducing the amount of logic that needs to be reasoned about.
We want to hide the actual buffer implementation from most of these locations. Currently it is a TransmitBuf but it likely will become something else in the future. So make this generic.
This allows making TransmitBuf::buf private at last. Now all the logic it handles is fully encapsulated.
This re-arranges the loop in poll_transmit to always finish the packet before going to the next iteration. This primarily enables to mutably borrow the TransmitBuf into a packet-specific buffer while the packet is being built. But this is not yet utilised in this commit. It does however remove the need of the mutable builder_storage Option, which makes reasoning over packet building slightly easier. - The logic to know on which packet space to send next, or whether there is no longer anything to send, has been moved to the next_send_space method. - The logic to decide whether to pad a packet before finishing it is moved to the end of the loop. - The logic to check the congestion controller and pacing is kept at the start of the loop. Before a new packet is started. Starting a new datagram also stays there.
Now that the packet is always finished at the end of the loop we no longer need to carry around the mutable SentFrames. Reducing further the number of things to keep track of.
The PacketBuilder::finish_and_track function took a SentFrames argument as an Option. However this was an artifact of the poll_transmit loop tracking it in an Option before, and it not being clear this was always Some when things were going correctly. Now all the callers clearly always pass this in so we can remove the Option.
Now the lifetimes allow for this the PacketBuilder can own the TransmitBuf. This is gives it more control over the buffer into which the packet can be written. This commit itself does nothing interesting with this yet. It merely moves the buffer ownership in a mechanical way. However, this enables future changes to reduce the use of offsets in so many places.
This moves keeping track of the available frame space to the packet builder. The available space is now encoded into the BufMut returned by PacketBuilder::frame_space_mut(). This removes the need for the BufLen trait in many of the places writing frames.
Now that the PacketBuilder::frame_space_mut exists the direct BufMut impl on it can be removed. Nothing external needs to directly write into the packet buffer outside of the frame space.
This trait is still the simplest way for the PartialEncode to keep track of the length of the header it just wrote.
It no longer needs to keep track of this field because the TransmitBuf already does. Removing duplicate state is nice.
This logic is internal business of the PacketBuilder. Use the provided abstraction.
This is duplicate information that is already available.
This starts removing the usage of absolute offsets in the TransmitBuf. It simplifies the resulting logic in the poll_transmit function. The absolute offsets are still available since the PacketBuilder still uses those.
You always need to remember to handle pad_datagram if needed. While really this always happens just before the call to finish_and_track. Instead this can be done in finish_and_track without any logic change, and this helps avoiding mistakes.
flub
changed the title
flub
added a commit
to n0-computer/quinn
that referenced
this pull request
Apr 11, 2025
This applies the refactors from quinn-rs#2168 and quinn-rs#2195 onto our multipath branch!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a continuation of #2168 and concentrates on further simplifying the logic needed in the poll_transmit function.
PacketBuilderwhile building a packet.BufMut::remaining_mutto know how much space is still available to write for frames. The intermediateBufLentrait is no longer needed for building packets.PacketBuilderto own the buffer, which is what allows usingBufMut::remaining_mutfor the buffer size..next_send_space()is easier to adapt to mutlipath. Though that's kind of out-of scope and not yet proven.The downside of using
BufMut::limitis thatbytes'Limit(and in generalBufMut) will panic if you try to write more than allowed. This is an invariant that should not occur, though in generalpoll_transmitcan not propagate errors or even close the connection. So while a custom trait could allow us to survive this we'd still build an invalid packet and then who knows what the remaining state of the connection would be. Perhaps it could be interesting to some day allow poll_transmit to return an error which would result in the connection being closed. But for now I think this choice ofBufMut::remaining_mutis fine.What's next?
The
TransmitBufstill exposes absolute offsets which are used by bothPacketBuilderandPartialEncode. I think the logic ofPacketBuilderandPartialEncodecould probably be further simplified by taking the same approach there and not use absolute offsets in theTransmitBuf. Though I've limited the scope of this PR to mainly improving poll_transmit, as this needs more maintenance that the packet building. Just as for #2168, this PR should not be judged on what could happen next.This PR is on top of #2168. Unfortunately I can't target that PR since the branch does not live in the quinn-rs/quinn repo. So you get the entire diff of #2168 and this together.
This PR starts at
Finish packets at end of each poll_transmit loop(currently e7ea038)