refactor: enable mozilla-central http3server to use neqo-bin #1878
Conversation
The QUIC Interop Runner requires an http3 and http09 implementation for both client and server. The client code is already structured into an http3 and an http09 implementation since mozilla#1727. This commit does the same for the server side, i.e. splits the http3 and http09 implementation into separate Rust modules.
There are two server implementations based on neqo: 1. https://github.com/mozilla/neqo/tree/main/neqo-bin/src/server - http3 and http09 implementation - used for manual testing and QUIC Interop 2. https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs - used to test Firefox I assume one was once an exact copy of the other. Both implement their own I/O, event loop, ... Since then, the two implementations diverged significantly. Especially (1) saw a lot of improvements in recent months: - mozilla#1564 - mozilla#1569 - mozilla#1578 - mozilla#1581 - mozilla#1604 - mozilla#1612 - mozilla#1676 - mozilla#1692 - mozilla#1707 - mozilla#1708 - mozilla#1727 - mozilla#1753 - mozilla#1756 - mozilla#1766 - mozilla#1772 - mozilla#1786 - mozilla#1787 - mozilla#1788 - mozilla#1794 - mozilla#1806 - mozilla#1808 - mozilla#1848 - mozilla#1866 At this point, bugs in (2) are hard to fix, see e.g. mozilla#1801. This commit merges (2) into (1), thus removing all duplicate logic and having (2) benefit from all the recent improvements to (1).
|
After reviewing the code in However, I do think that extracting common elements such as I/O, timers, and the event loop to simplify the code in http3server would be nice. |
|
|
Yeah, I think this sounds good to me. |
|
Status update:
Still needs a bit of clean-up. Will create my first mozilla-central patch soon. |
|
I have created a accompanying Bugzilla Bug and added a Phabricator patch. I suggest we keep high level discussions here on this GitHub pull request. |
|
The merged One potential problem is that we need to vendor some third-party rust crates into mozilla-central, as shown in this link |
…-central-http3-server
mxinden
changed the title
I will take a look. Thanks @KershawChang! |
Nice to have. Adds multiple dependencies. Hard to justify for mozilla-central.
|
955aaa9 should drop the need for the following dependencies: Which leaves us with the following additional dependencies for mozilla central: I don't think we can nor do we want to do without Must have for Could be removed through some semi-intrusive feature flags in |
Benchmark resultsPerformance differences relative to beffcc6.
Client/server transfer resultsTransfer of 134217728 bytes over loopback.
|
Aside: It looks like we will need to consider alternative options at some point, because |
|
@larseggert yes, unfortunately. Commit was part of quinn-rs/quinn#1729. I reviewed the pull request before that commit only (quinn-rs/quinn#1729 (review)). (The pull request has another change which is beneficial for I am not sure this was a deliberate choice against
Agreed. #1693 is currently paused in favor of #1801 and this pull request. Also I believe #1868 is a lower hanging fruit for now. Long story short, I don't think we need to make a decision today. |
We definitely don't. |
|
The two "Firefox / Build Firefox" failures seem unrelated and seem to just have timed out. @larseggert fine to ignore for now? Edit: Probably related #1893. |
|
If @KershawChang is happy, I am good to land this. |
Looks good to me. I think we can land this. |
…lla#1800) This reverts commit 342e4e7. With mozilla#1878 merged and https://bugzilla.mozilla.org/show_bug.cgi?id=1895319 available, one can now reapply the patch removing `Server::timers`. More specifically, the actual bug fix on mozilla-central side: ``` rust let output = if self.response_to_send.is_empty() { output } else { // In case there are pending responses to send, make sure a reasonable // callback is returned. const MIN_INTERVAL: Duration = Duration::from_millis(100); match output { Output::None => Output::Callback(MIN_INTERVAL), o @ Output::Datagram(_) => o, Output::Callback(d) => Output::Callback(min(d, MIN_INTERVAL)), } }; ``` See https://phabricator.services.mozilla.com/D209574.
This reverts commit 342e4e7. With #1878 merged and https://bugzilla.mozilla.org/show_bug.cgi?id=1895319 available, one can now reapply the patch removing `Server::timers`. More specifically, the actual bug fix on mozilla-central side: ``` rust let output = if self.response_to_send.is_empty() { output } else { // In case there are pending responses to send, make sure a reasonable // callback is returned. const MIN_INTERVAL: Duration = Duration::from_millis(100); match output { Output::None => Output::Callback(MIN_INTERVAL), o @ Output::Datagram(_) => o, Output::Callback(d) => Output::Callback(min(d, MIN_INTERVAL)), } }; ``` See https://phabricator.services.mozilla.com/D209574.
There are two server implementations based on neqo:
https://github.com/mozilla/neqo/tree/main/neqo-bin/src/server
https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs
I assume one was once an exact copy of the other. Both implement their own I/O, timers, event loop, ... Since then, the two implementations diverged significantly. Especially (1) saw a lot of improvements in recent months:
At this point, bugs in (2) are hard to fix, see e.g. #1801.
This commit merges (2) into (1), thus removing all duplicate logic and having (2) benefit from all the recent improvements to (1).
Opening as a draft for now. Blocked on #1877.
What do folks think? Are you in favor of merging the two? We can then discuss details, e.g. havingfirefox.rswith its variousHttpServerimplementations live in mozilla-central instead of GitHub.