quinn icon indicating copy to clipboard operation
quinn copied to clipboard

Make PacketBuilder own the write buffer

Open flub opened this issue 6 months ago • 1 comments

This is a continuation of #2168 and concentrates on further simplifying the logic needed in the poll_transmit function.

  • The buffer to write into is now owned by the PacketBuilder while building a packet.
  • This allows using BufMut::remaining_mut to know how much space is still available to write for frames. The intermediate BufLen trait is no longer needed for building packets.
  • The poll_transmit function and it's helpers that write frames now no longer deals with offsets into an abstract buffer. It removes the amount of state they need to track and the logical conditions end up being simpler.
  • The main loop has been modified: the goal here was to finish the packet before the loop re-started.
    • This allows the PacketBuilder to own the buffer, which is what allows using BufMut::remaining_mut for the buffer size.
    • It also reduces the amount of state that needs to be carried between loop iterations. Which reduces the cognitive load for readers.
    • Finally, my hope is that this loop construction with .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::limit is that bytes' Limit (and in general BufMut) will panic if you try to write more than allowed. This is an invariant that should not occur, though in general poll_transmit can 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 of BufMut::remaining_mut is fine.

What's next?

The TransmitBuf still exposes absolute offsets which are used by both PacketBuilder and PartialEncode. I think the logic of PacketBuilder and PartialEncode could probably be further simplified by taking the same approach there and not use absolute offsets in the TransmitBuf. 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 e7ea03832f5ad7dc29067899efb94b068d83e06b)

flub avatar Apr 03 '25 09:04 flub