fix(bin/client): don't close closing connection #1866
Conversation
The `bin/src/client/mod.rs` `Runner::run` function continuously checks whether there is more work. In case there is none, it initiates closing of the connection (`self.client.close`) and then `continue`s to the top of the loop in order to send out a closing frame. https://github.com/mozilla/neqo/blob/14cafbaa7fa88434def2c1d19e932c08e00173f8/neqo-bin/src/client/mod.rs#L376-L409 There is a potential busy loop when closing an already closing connection. `Runner::run` will call `self.client.close` and then continously `continue` to the top of the loop. This commit differentiates a connection state in `NotClosing`, `Closing` and `Closed`. It only attempts to close a `NotClosing` connection and only then `continue`s to the top of the loop.
mxinden
requested review from
KershawChang,
martinthomson and
larseggert
as code owners
|
Any chance we can refactor this to avoid the duplication between h09 and h3? |
Head branch was pushed to by a user without write access
|
@larseggert @martinthomson the latest commit does the following:
I think it is an improvement. I don't think it is great. Let me know what you think. |
Benchmark resultsPerformance differences relative to 8c4411a.
Client/server transfer resultsTransfer of 134217728 bytes over loopback.
|
That might be because |
The `neqo_transport::ConnectionError` enum contains the two non-error variants `Error::NoError` and `CloseReason::Application(0)`. In other words, `ConnectionError` contains variants that are not errors. This commit renames `ConnectionError` to the more descriptive name `CloseReason`. See suggestion in mozilla#1866 (comment). To ease the upgrade for downstream users, this commit adds a deprecated `ConnectionError`, guiding users to rename to `CloseReason` via a deprecation warning. ``` rust pub type ConnectionError = CloseReason; ```
The `neqo_transport::ConnectionError` enum contains the two non-error variants `Error::NoError` and `CloseReason::Application(0)`. In other words, `ConnectionError` contains variants that are not errors. This commit renames `ConnectionError` to the more descriptive name `CloseReason`. See suggestion in mozilla#1866 (comment). To ease the upgrade for downstream users, this commit adds a deprecated `ConnectionError`, guiding users to rename to `CloseReason` via a deprecation warning. ``` rust pub type ConnectionError = CloseReason; ```
The `neqo_transport::ConnectionError` enum contains the two non-error variants `Error::NoError` and `CloseReason::Application(0)`. In other words, `ConnectionError` contains variants that are not errors. This commit renames `ConnectionError` to the more descriptive name `CloseReason`. See suggestion in #1866 (comment). To ease the upgrade for downstream users, this commit adds a deprecated `ConnectionError`, guiding users to rename to `CloseReason` via a deprecation warning. ``` rust pub type ConnectionError = CloseReason; ```
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).
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).
* 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.
The
bin/src/client/mod.rsRunner::runfunction continuously checks whether there is more work. In case there is none, it initiates closing of the connection (self.client.close) and thencontinues to the top of the loop in order to send out a closing frame.neqo/neqo-bin/src/client/mod.rs
Lines 376 to 409 in 14cafba
There is a potential busy loop when closing an already closing connection.
Runner::runwill callself.client.closeand then continouslycontinueto the top of the loop.This commit differentiates a connection state in
NotClosing,ClosingandClosed. It only attempts to close aNotClosingconnection and only thencontinues to the top of the loop.Fixes #1864.