quinn-rs / quinn

Async-friendly QUIC implementation in Rust
Apache License 2.0
3.85k stars 394 forks source link

feat: use mutable transmits to AsyncUdpSocket::poll_send #1692

Closed dignifiedquire closed 7 months ago

dignifiedquire commented 1 year ago

In our implementation (n0-computer/iroh) we currently have to maniuplate the destination addresses based on internal mappings. Currently we need to copy the full Transmit slice in order to update these addresses. To avoid this copy, this changes it such that a mutable reference is passed in.

Ralith commented 1 year ago

What happens when a poll_send call isn't able to transmit all packets immediately, i.e. returns less than transmits.len()? It seems difficult to avoid unpredictably double-translating addresses with this change, which could be harmful if your translation isn't idempotent.

dignifiedquire commented 1 year ago

That should be easy enough to handle on our side, we can either reset to the original addrs for those that weren't sent, or detect based on the addr if we have mapped it yet.

dignifiedquire commented 1 year ago

The CI failures look unrelated to me: BSD seems to hang and the lint is complaining about unrelated code.

chayleaf commented 11 months ago

this would be useful for us too (for address mangling as well)

Ralith commented 11 months ago

I'd like to at least see a clearly documented contract for whether or not unsent entries may be changed after the call returns. I think it should specifically guarantee that they won't be to avoid unpredictable behavior. It's not clear to me if that guarantee can be respected without defeating the point by requiring storage on the side, though.

Is this copy showing up in profiling at all? It should be straightforward to reuse an allocation for it.

Ralith commented 11 months ago

https://github.com/quinn-rs/quinn/pull/1729 will likely render this unnecessary by only accepting a single, trivially rewritten Transmit at a time.

dignifiedquire commented 7 months ago

Closing in favor of #1729