refactor(client): de-duplicate process & run and split into h3 & h09 #1727

mxinden · 2024-03-10T20:29:13Z

The Neqo Client binary supports both http3 and http09 (prev. "old").

Before this commit both the http3 and the http09 implementation had their own run and process fn, orchestrating the interaction between handler, client and I/O. While similar, they had subtle differences e.g. when to terminate.

This commit splits the http3 and http09 specific logic into two separate modules, but extracts duplicate logic (e.g. run and process) into the shared root module.

For reviewers: I suggest reviewing the two commits individually. The first simply moves bin/client.rs to bin/client/main.rs. It is done in a separate commit in order for git to recognize the move as a move and not as a remove and an add, thus showing a proper diff.

Part of #1696.

Simplifying process and run will make #1693 easier to implement.

codecov · 2024-03-10T20:46:27Z

Codecov Report

Attention: Patch coverage is 0% with 808 lines in your changes are missing coverage. Please review.

Project coverage is 89.48%. Comparing base (ce5cbe4) to head (49ce1ae).

Files	Patch %	Lines
neqo-bin/src/bin/client/main.rs	0.00%	325 Missing ⚠️
neqo-bin/src/bin/client/http3.rs	0.00%	299 Missing ⚠️
neqo-bin/src/bin/client/http09.rs	0.00%	184 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1727      +/-   ##
==========================================
+ Coverage   89.41%   89.48%   +0.07%     
==========================================
  Files         124      126       +2     
  Lines       38897    38866      -31     
==========================================
  Hits        34780    34780              
+ Misses       4117     4086      -31

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

As a preparation to introducing sub-modules to `client`. Done in a separate commit for git to recognize the move as a move, not as a remove and add.

The Neqo Client binary supports both http3 and http09 (prev. "old"). Before this commit both the http3 and the http09 implementation had their own `run` and `process` `fn`, orchestrating the interaction between handler, client and I/O. While similar, they had subtle differences e.g. when to terminate. This commit splits the http3 and http09 specific logic into two separate modules, but extracts duplicate logic (e.g. `run` and `process`) into the shared root module.

mxinden · 2024-03-11T08:06:13Z

neqo-bin/src/bin/client.rs

For reviewers: I suggest reviewing the two commits individually. The first simply moves bin/client.rs to bin/client/main.rs. It is done in a separate commit in order for git to recognize the move as a move and not as a remove and an add, thus showing a proper diff.

mxinden · 2024-03-11T08:07:06Z

neqo-bin/src/bin/client/main.rs

+}
+
+impl<'a, H: Handler> Runner<'a, H> {
+    async fn run(mut self) -> Res<Option<ResumptionToken>> {


With this commit, the Neqo client implementation has a single run function, not one for http3 and one for http09.

mxinden · 2024-03-11T08:08:13Z

neqo-bin/src/bin/client/main.rs

+            token = if args.shared.use_old_http {
+                let client =
+                    http09::create_client(&args, real_local, remote_addr, &hostname, token)
+                        .expect("failed to create client");
+
+                let handler = http09::Handler::new(to_request, &args, key_update);
+
+                Runner {
+                    args: &args,
+                    client,
+                    handler,
+                    local_addr: real_local,
+                    socket: &mut socket,
+                    timeout: None,
+                }
+                .run()
+                .await?
+            } else {
+                let client = http3::create_client(&args, real_local, remote_addr, &hostname, token)
+                    .expect("failed to create client");
+
+                let handler = http3::Handler::new(to_request, &args, key_update);
+
+                Runner {
+                    args: &args,
+                    client,
+                    handler,
+                    local_addr: real_local,
+                    socket: &mut socket,
+                    timeout: None,
+                }
+                .run()
+                .await?
+            };


http3 and http09 expose the same methods (create_client and Handler::new) and can be run with the same Runner.

…rge-run

mxinden · 2024-03-13T11:26:50Z

Friendly ping. Any objections? If not, @larseggert would you mind adding this pull request to the merge queue?

github-actions · 2024-03-14T00:30:24Z

Benchmark results

coalesce_acked_from_zero 1+1 entries
time: [255.61 ns 256.09 ns 256.59 ns]
change: [+1.6942% +2.0998% +2.4502%] (p = 0.00 < 0.05)
💔 Performance has regressed.
coalesce_acked_from_zero 3+1 entries
time: [292.21 ns 294.83 ns 299.39 ns]
change: [+1.7800% +2.4496% +3.4445%] (p = 0.00 < 0.05)
💔 Performance has regressed.
coalesce_acked_from_zero 10+1 entries
time: [290.92 ns 291.96 ns 293.18 ns]
change: [+1.3610% +2.0948% +2.8228%] (p = 0.00 < 0.05)
💔 Performance has regressed.
coalesce_acked_from_zero 1000+1 entries
time: [267.45 ns 267.70 ns 267.98 ns]
change: [+2.9024% +3.6610% +4.4035%] (p = 0.00 < 0.05)
💔 Performance has regressed.
RxStreamOrderer::inbound_frame()
time: [125.60 ms 125.68 ms 125.77 ms]
change: [+2.9056% +2.9958% +3.0804%] (p = 0.00 < 0.05)
💔 Performance has regressed.
Run multiple transfers with varying seeds
time: [187.93 ms 188.60 ms 189.28 ms]
change: [-6.8859% -6.4260% -5.9650%] (p = 0.00 < 0.05)
💚 Performance has improved.
Run multiple transfers with the same seed
time: [189.75 ms 190.17 ms 190.64 ms]
change: [-5.1767% -4.8874% -4.5829%] (p = 0.00 < 0.05)
💚 Performance has improved.

⬇️ Download logs

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).

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).

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 #1727. This commit does the same for the server side, i.e. splits the http3 and http09 implementation into separate Rust modules.

* refactor(bin): introduce server/http3.rs and server/http09.rs 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 #1727. This commit does the same for the server side, i.e. splits the http3 and http09 implementation into separate Rust modules. * refactor: merge mozilla-central http3 server into neqo-bin 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: - #1564 - #1569 - #1578 - #1581 - #1604 - #1612 - #1676 - #1692 - #1707 - #1708 - #1727 - #1753 - #1756 - #1766 - #1772 - #1786 - #1787 - #1788 - #1794 - #1806 - #1808 - #1848 - #1866 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). * Move firefox.rs to mozilla-central * Reduce HttpServer trait functions * Extract constructor * Remove unused deps * Remove clap color feature Nice to have. Adds multiple dependencies. Hard to justify for mozilla-central.

mxinden force-pushed the client-merge-run branch from 6f4b243 to a0236db Compare March 11, 2024 07:48

mxinden added 2 commits March 11, 2024 08:54

refactor(bin): move client.rs to client/main.rs

ae73e19

As a preparation to introducing sub-modules to `client`. Done in a separate commit for git to recognize the move as a move, not as a remove and add.

mxinden force-pushed the client-merge-run branch from a0236db to 721f3e1 Compare March 11, 2024 07:56

mxinden commented Mar 11, 2024

View reviewed changes

mxinden marked this pull request as ready for review March 11, 2024 08:08

mxinden requested review from KershawChang, martinthomson and larseggert as code owners March 11, 2024 08:08

larseggert approved these changes Mar 11, 2024

View reviewed changes

Merge branch 'main' of https://github.com/mozilla/neqo into client-me…

d091f16

…rge-run

mxinden mentioned this pull request Mar 12, 2024

refactor(udp): move udp mod from neqo-common to neqo-bin #1736

Merged

larseggert enabled auto-merge March 13, 2024 23:45

Merge branch 'main' into client-merge-run

49ce1ae

larseggert disabled auto-merge March 14, 2024 00:45

larseggert merged commit 203987a into mozilla:main Mar 14, 2024
14 of 15 checks passed

mxinden mentioned this pull request May 3, 2024

refactor(bin): introduce server/http3.rs and server/http09.rs #1877

Merged

mxinden mentioned this pull request May 4, 2024

refactor: enable mozilla-central http3server to use neqo-bin #1878

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(client): de-duplicate process & run and split into h3 & h09 #1727

refactor(client): de-duplicate process & run and split into h3 & h09 #1727

mxinden commented Mar 10, 2024 •

edited

Loading

codecov bot commented Mar 10, 2024 •

edited

Loading

mxinden Mar 11, 2024

mxinden Mar 11, 2024

mxinden Mar 11, 2024

mxinden commented Mar 13, 2024

github-actions bot commented Mar 14, 2024

refactor(client): de-duplicate process & run and split into h3 & h09 #1727

refactor(client): de-duplicate process & run and split into h3 & h09 #1727

Conversation

mxinden commented Mar 10, 2024 • edited Loading

codecov bot commented Mar 10, 2024 • edited Loading

Codecov Report

mxinden Mar 11, 2024

Choose a reason for hiding this comment

mxinden Mar 11, 2024

Choose a reason for hiding this comment

mxinden Mar 11, 2024

Choose a reason for hiding this comment

mxinden commented Mar 13, 2024

github-actions bot commented Mar 14, 2024

Benchmark results

mxinden commented Mar 10, 2024 •

edited

Loading

codecov bot commented Mar 10, 2024 •

edited

Loading