Add net.TCPConnection & remove TCPIPConnection #1414
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
plajjan
force-pushed
the
net-tcpconnection
branch
from
17bc5cf
to
6e900ef
Compare
This adds a new TCP client connection actor which replaces the older TCPIPConnection actor. It improves on the old implementation through the addition of DNS resolution and Happy Eyeballs (RFC6555). While not super complex it is non-trivial compared to the old connection actor and so there are probably bugs lurking. More test cases are needed. Right now we do a rather aggressive Happy Eyeballs where we instantly connect both to IPv4 and IPv6. This effectively doubles the resources consumed, fds on our end and more on the server side. While the RFC allows for this, the recommendation is to have a slight delay for the IPv4 side, like I think Chrome does 300ms or so. We can look into that. Added net.is_ipv4() and net.is_ipv6() as well. Originally had ideas about just adding this and letting the older TCPIPConnection remain but as we've made backwards incompatible changes anyway, it's just easier to replace it right away.
plajjan
force-pushed
the
net-tcpconnection
branch
from
6e900ef
to
0a59515
Compare
plajjan
commented
Aug 14, 2023
Need to exit *after* we've printed the output, duh, what a typo.
A connection now times out after 10 seconds after which the on_error callback will be called. 10 seconds seems fairly long but mobile clients (or from an airplane!) or similar could sometimes suffer from very long latency and better late than never?
The TCPListener can now listen to an IPv4 or IPv6 address! Added a test case for it as well but it is unfortunately disabled since the CI environment currently does not support IPv6 (GitHub Actions do not do IPv6 for docker containers).
plajjan
force-pushed
the
net-tcpconnection
branch
from
52b11b7
to
8f3d8f1
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a new TCP client connection actor which replaces the older TCPIPConnection actor. It improves on the old implementation through the addition of DNS resolution and Happy Eyeballs (RFC6555).
While not super complex it is non-trivial compared to the old connection actor and so there are probably bugs lurking. More test cases are needed.
Right now we do a rather aggressive Happy Eyeballs where we instantly connect both to IPv4 and IPv6. This effectively doubles the resources consumed, fds on our end and more on the server side. While the RFC allows for this, the recommendation is to have a slight delay for the IPv4 side, like I think Chrome does 300ms or so. We can look into that.
Added net.is_ipv4() and net.is_ipv6() as well.