Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Redo #1784 #1801

Closed
larseggert opened this issue Apr 9, 2024 · 8 comments
Closed

Redo #1784 #1801

larseggert opened this issue Apr 9, 2024 · 8 comments

Comments

@larseggert
Copy link
Collaborator

@KershawChang said in #1800:

It appears that PR #1784 caused some test failures (failed tests on windows). After reverting it, all http3 tests are passed.

We should find out why those treeherder tests failed, and write a neqo unittest that replicates that failure. And then take another stab at #1784.
@mxinden
Copy link
Collaborator

mxinden commented Apr 9, 2024

Sorry for the trouble @KershawChang.

Do I understand correctly, that the failing tests all use https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs?

@KershawChang
Copy link
Collaborator

Do I understand correctly, that the failing tests all use https://searchfox.org/mozilla-central/source/netwerk/test/http3server/src/main.rs?

Yes, that's correct.
Note that those tests are only failed on windows platform, so I assume this could be a timing issue, which makes it not easy to debug. Or it could be the fault of our flaky test server that needs to be modified.

@mxinden
Copy link
Collaborator

mxinden commented Apr 12, 2024

If I understand correctly the GitHub HTTP3 server implementation (neqo-bin/server/mod.rs) and the mozilla-central HTTP3 server implementation have a lot in common, e.g.:

  • wrapper logic around neqo-http3 driving neqo_http3::Http3Server
  • UDP I/O logic

What is the long term plan for the two? Should they simply co-exist as they do today? Should they merge? Should they share components?

@larseggert
Copy link
Collaborator Author

larseggert commented Apr 15, 2024 •
edited
Loading

Good topic to discuss on the next call. I don't have history of why the current state is as it is, but I'd personally like to reduce duplication as much as possible, and ideally have most of the logic in the neqo repo where we can more easily update and test it.

(We should probably add a CI step to build Fx with PRs that propose changes to neqo, and ideally also execute some Fx-level tests.)

@KershawChang
Copy link
Collaborator

What is the long term plan for the two? Should they simply co-exist as they do today? Should they merge? Should they share components?

It's probably not easy to merge the two, but sharing some components and reducing the duplicate code definitely makes sense to me.

@mxinden
Copy link
Collaborator

mxinden commented Apr 15, 2024

Good topic to discuss on the next call.

Added to the agenda document ✔️.

@KershawChang
Copy link
Collaborator

I’ve investigated this issue and believe it is caused by a design flaw in our test http3server. Specifically, with the changes introduced in #1784, the Http3Server::process method sometimes returns Output::None. This behavior is problematic for the http3server, which depends on the process_datagrams_and_events method being called periodically. However, process_datagrams_and_events is only triggered when a timer expires or a packet is received. This could lead to stalls.

I think the right fix would be to refactor the http3server such that process_datagrams_and_events is called when there is data to send.

@mxinden mxinden mentioned this issue Apr 23, 2024
Merged
mxinden added a commit to mxinden/neqo that referenced this issue May 4, 2024
@mxinden
refactor: merge mozilla-central http3 server into neqo-bin
98b969e 
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).
@mxinden mxinden mentioned this issue May 4, 2024
Merged
KershawChang pushed a commit to KershawChang/neqo that referenced this issue May 7, 2024
@mxinden @KershawChang
refactor: merge mozilla-central http3 server into neqo-bin
d1900d6 
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).
github-merge-queue bot pushed a commit that referenced this issue May 8, 2024
@mxinden
refactor: enable mozilla-central http3server to use neqo-bin (#1878)
b8ae981 
* 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
Copy link
Collaborator

mxinden commented May 27, 2024

With #1878 merged and https://bugzilla.mozilla.org/show_bug.cgi?id=1895319 closed, this issue can be closed as well. Would you mind @larseggert?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@larseggert @KershawChang @mxinden