feat(bin): enable Firefox to use quinn-udp through neqo-udp #1920
Conversation
mxinden
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1920 +/- ##
==========================================
- Coverage 94.82% 94.80% -0.02%
==========================================
Files 110 111 +1
Lines 35792 35968 +176
==========================================
+ Hits 33938 34100 +162
- Misses 1854 1868 +14 ☔ View full report in Codecov by Sentry. |
mxinden
requested review from
KershawChang,
martinthomson and
larseggert
as code owners
There was a problem hiding this comment.
Looks good to me.
@martinthomson , @larseggert , could you also review this PR?
Thanks.
neqo-udp/Cargo.toml
Outdated
| [dev-dependencies] | ||
| tokio = { version = "1", default-features = false, features = ["net", "time", "macros", "rt", "rt-multi-thread"] } |
There was a problem hiding this comment.
Why is tokio also a dev-dependency?
There was a problem hiding this comment.
Presumably because the dev variant has more features enabled.
The lack of an EOL here is also worth fixing. You can configure your editor to add these to all text files.
There was a problem hiding this comment.
Correct.
Firefox does not need tokio. neqo-client and neqo-server in neqo-bin do need tokio, but only the net feature. Thus tokio is an optional dependency restricted to the net feature only.
[dependencies]
# [...]
tokio = { version = "1", default-features = false, features = ["net"], optional = true }In addition, the neqo-udp tests always need tokio and they make use of various features beyond net, thus the additional import as a dev-dependency with more features enabled.
[dev-dependencies]
tokio = { version = "1", default-features = false, features = ["net", "time", "macros", "rt", "rt-multi-thread"] }
The lack of an EOL here is also worth fixing. You can configure your editor to add these to all text files.
Will do!
| use quinn_udp::{EcnCodepoint, RecvMeta, Transmit, UdpSocketState}; | ||
| use tokio::io::Interest; | ||
|
|
||
| /// Socket receive buffer size. | ||
| /// | ||
| /// Allows reading multiple datagrams in a single [`Socket::recv`] call. | ||
| const RECV_BUF_SIZE: usize = u16::MAX as usize; |
There was a problem hiding this comment.
This is the max on all platforms?
There was a problem hiding this comment.
That seems unlikely. But @larseggert, does that need to be fixed here, or can we track that with another issue?
There was a problem hiding this comment.
I think finding the right value needs more experimentation.
Here is what others use:
Google's Quiche handles 1472 bytes per segment with at most 16 segments per read.
MSQuic uses (UINT16_MAX - CXPLAT_UDP_HEADER_SIZE).
Quinn uses a receive buffer of 64 * 1024.
Note that in Firefox this buffer is allocated once per socket thread. Given that Firefox has a single socket thread only, there is effectively a single u16::MAX receive buffer only.
Would you recommend a larger buffer size to start with @larseggert?
neqo-udp/src/lib.rs
Outdated
| meta.addr, | ||
| *local_address, | ||
| meta.ecn.map(|n| IpTos::from(n as u8)).unwrap_or_default(), | ||
| None, // TODO: get the real TTL https://github.com/quinn-rs/quinn/issues/1749 |
There was a problem hiding this comment.
I wonder if we should remove the TTL stuff from neqo until quinn actually supports it...
There was a problem hiding this comment.
Separate issue, but I'm not sure that we really need TTL, so I'd be happy to see it removed.
There was a problem hiding this comment.
Sounds good to me. I created #1940.
neqo-udp/Cargo.toml
Outdated
| [dev-dependencies] | ||
| tokio = { version = "1", default-features = false, features = ["net", "time", "macros", "rt", "rt-multi-thread"] } |
There was a problem hiding this comment.
Presumably because the dev variant has more features enabled.
The lack of an EOL here is also worth fixing. You can configure your editor to add these to all text files.
| use quinn_udp::{EcnCodepoint, RecvMeta, Transmit, UdpSocketState}; | ||
| use tokio::io::Interest; | ||
|
|
||
| /// Socket receive buffer size. | ||
| /// | ||
| /// Allows reading multiple datagrams in a single [`Socket::recv`] call. | ||
| const RECV_BUF_SIZE: usize = u16::MAX as usize; |
There was a problem hiding this comment.
That seems unlikely. But @larseggert, does that need to be fixed here, or can we track that with another issue?
neqo-udp/src/lib.rs
Outdated
| meta.addr, | ||
| *local_address, | ||
| meta.ecn.map(|n| IpTos::from(n as u8)).unwrap_or_default(), | ||
| None, // TODO: get the real TTL https://github.com/quinn-rs/quinn/issues/1749 |
There was a problem hiding this comment.
Separate issue, but I'm not sure that we really need TTL, so I'd be happy to see it removed.
mozilla#1568 introduced TTL information to datagrams. On the input path it would take the ttl information, but not act on it. On the output path it would set only "the default TTL on many OSes". https://github.com/mozilla/neqo/blob/66504908e5fa070a8a5fa67d8b5a201d2c9a5cc5/neqo-transport/src/path.rs#L576 This commit removes the ttl information from `Datagram`, thus reverting a subset of mozilla#1568. This is partially motivated by mozilla#1920, introducing `quinn-udp` as the IO library of choice, which does not support reading and writing TTL. See also discussion in mozilla#1920 (comment).
mozilla#1568 introduced TTL information to datagrams. On the input path it would take the ttl information, but not act on it. On the output path it would set only "the default TTL on many OSes". https://github.com/mozilla/neqo/blob/66504908e5fa070a8a5fa67d8b5a201d2c9a5cc5/neqo-transport/src/path.rs#L576 This commit removes the ttl information from `Datagram`, thus reverting a subset of mozilla#1568. This is partially motivated by mozilla#1920, introducing `quinn-udp` as the IO library of choice, which does not support reading and writing TTL. See also discussion in mozilla#1920 (comment).
#1568 introduced TTL information to datagrams. On the input path it would take the ttl information, but not act on it. On the output path it would set only "the default TTL on many OSes". https://github.com/mozilla/neqo/blob/66504908e5fa070a8a5fa67d8b5a201d2c9a5cc5/neqo-transport/src/path.rs#L576 This commit removes the ttl information from `Datagram`, thus reverting a subset of #1568. This is partially motivated by #1920, introducing `quinn-udp` as the IO library of choice, which does not support reading and writing TTL. See also discussion in #1920 (comment).
neqo-udpcrate. Firefox does not needneqo-server,neqo-client, nor any of its dependencies. A new crate enforces none of this leaking into Firefox.Socketto be used both withtokio::net::UdpSocket(forneqo-client,neqo-server, mozilla-central'shttp3server) andstd::os::fd::BorrowedFd(std::os::windows::io::BorrowedSocketon Windows). The latter allowsneqo-glueto useneqo-udpwith a UDP socket managed byNSPR.