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.

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

feat(ext/net): add reuseAddress option for UDP #13849

Merged
merged 1 commit into from Oct 24, 2022
Merged

feat(ext/net): add reuseAddress option for UDP #13849

merged 1 commit into from Oct 24, 2022

Conversation

Trolloldem
Copy link
Contributor

@Trolloldem Trolloldem commented Mar 6, 2022

This commit adds a reuseAddress option for UDP sockets. When this option is enabled, one can listen on an address even though it is already being listened on from a different process or thread. The new socket will steal the address from the existing socket.

On Windows and Linux this uses the SO_REUSEADDR option, while on other Unixes this is done with SO_REUSEPORT.

This behavior aligns with what libuv does.

TCP sockets still unconditionally set the SO_REUSEADDR flag - this behavior matches Node.js and Go. This PR does not change this behaviour.

@Trolloldem
Copy link
Contributor Author

I think there is something wrong with this PR. The CI keep failing even after several resets, but when I run the same CI after a merge of the branch on my main branch, which starts from a clean state, they are successful. Am I doing something wrong in the process?

@bartlomieju
Copy link
Member

@Trolloldem you are hitting an internal compiler error in Rust. You can (most likely) fix this, by changing cache keys in .github/workflows/ci.yml - 5-cargo-... to 6-cargo-...

@Trolloldem
Copy link
Contributor Author

Thank you @bartlomieju, it seems fine now. Is it better to also add test for the described behavior in Windows?

@CLAassistant
Copy link

CLAassistant commented Mar 10, 2022

CLA assistant check
All committers have signed the CLA.

@bartlomieju
Copy link
Member

Thank you @bartlomieju, it seems fine now. Is it better to also add test for the described behavior in Windows?

That would be great! I'll try to get this landed for 1.20

@Trolloldem
Copy link
Contributor Author

Thank you @bartlomieju, it seems fine now. Is it better to also add test for the described behavior in Windows?

That would be great! I'll try to get this landed for 1.20

Test for Windows behavior added and CI seem fine. I leave here a quick reference about the tests implemented:

  • netTcpReuseAddr opens a socket using the default setting for SO_REUSEADDR of each OS, then the socket is closed,
    putting it in the TIME_WAIT state; a second socket is successfully opened
  • netTcpNoReuseAddr performs the same steps of the previous test but SO_REUSEADDR is set to false, in order to test the behavior in OS different from Windows. In this case the test has to fail with an AddrInUse error. This test can fail in two point, proving the same property, the first point being the first listener creation (it can fail since sockets from previous tests that occupied the required port are in the TIME_WAIT state) while the second point is the creation of the second listener (since the first one is in the TIME_WAIT state)
  • netTcpTrueReuseAddrWindows set SO_REUSEADDR to true and is run only in Windows. In this case two listeners are bound to the same port, which is usually forbidden for the default setting of SO_REUSEADDR equal to false

The tests listenTlsWithReuseAddr, listenTlsWithNoReuseAddr and listenTlsWithTrueReuseAddrWindows follow the same
logic but are for listenTls

@Trolloldem Trolloldem requested a review from AaronO as a code owner March 22, 2022 13:59
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trolloldem sorry for the delay, I've got two nitpicks and after that we can land the PR.

ext/net/lib.deno_net.d.ts Outdated Show resolved Hide resolved
ext/net/lib.deno_net.d.ts Outdated Show resolved Hide resolved
@Trolloldem
Copy link
Contributor Author

@Trolloldem sorry for the delay, I've got two nitpicks and after that we can land the PR.

No problem to me. I've noticed there was a lot of things moving with 1.20, so I took the chance to rearrange the commit history of the PR into a single commit + one commit to change the cache key.

I've addressed the requests concerning the docstrings and merged my branch with the latest version of the main branch of Deno

ext/net/lib.deno_net.d.ts Outdated Show resolved Hide resolved
@@ -76,6 +76,9 @@ declare namespace Deno {
* You should show the message like `server running on localhost:8080` instead of
* `server running on 0.0.0.0:8080` if your program supports Windows. */
hostname?: string;
/** SO_REUSEADDR option for socket.
* If not specified, defaults to true in Linux, false in Windows. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why default to true? Does Node default to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SO_REUSEADDR defaults to true since, as stated in #13808, the previous implementation of TCP socket used the std::net implementation, which sets SO_REUSEADDR to true when binding a TcpListener outside of Windows. In this way it is possible to keep the behavior of existing code that does not explicit use the new reuseAddress argument. Setting this to false as default value could break existing code due to the fact that in Unix system is not possible to bind socket to a combination of address+port occupied by closed sockets in the TIME_WAIT state, as visible in failed tests in #13794. In addition, this seem the same behavior of node: unix tcp, win tcp.

@Trolloldem
Copy link
Contributor Author

The changes introduced up to now concern only TCP sockets. With the last two commits I have extended the possibility of using the SO_REUSEADDR even for UDP sockets. Since they have a different behavior compared to TCP sockets, I have also updated the docstring of the reuseAddress optional argument. This argument now can be specified through Deno.listenDatagram. In addition to that, 2 new test have been added: netUdpReuseAddr and netUdpNoReuseAddr.

To enable this option, the Socket2 crate has been used, using the same settings of the std::net implementation of UdpSocket.

Similarly to node (win udp, unix udp), the options always defaults to false, and for both Windows and Linux setting SO_REUSEADDR actually uses that option under the hood. For the other *nix systems SO_REUSEPORT is actually used.

@Trolloldem
Copy link
Contributor Author

The changes introduced up to now concern only TCP sockets. With the last two commits I have extended the possibility of using the SO_REUSEADDR even for UDP sockets. Since they have a different behavior compared to TCP sockets, I have also updated the docstring of the reuseAddress optional argument. This argument now can be specified through Deno.listenDatagram. In addition to that, 2 new test have been added: netUdpReuseAddr and netUdpNoReuseAddr.

To enable this option, the Socket2 crate has been used, using the same settings of the std::net implementation of UdpSocket.

Similarly to node (win udp, unix udp), the options always defaults to false, and for both Windows and Linux setting SO_REUSEADDR actually uses that option under the hood. For the other *nix systems SO_REUSEPORT is actually used.

@bartlomieju sorry for the ping. Should I change the title after these changes (since now also UDP should be covered). In addition, is this still a requested feature? Should I provide any additional documentation?

@bartlomieju
Copy link
Member

@littledivy @lucacasonato could you take a look at this PR?

@lucacasonato
Copy link
Member

I have rebased this PR.

I have removed the reuseAddress option for TCP sockets. For TCP, this option is unconfigurably true on all platforms but Windows. This behaviour matches Node.js and Go. Neither of these systems support disabling SO_REUSEADDR on socket listeners.

For UDP, reuseAddress is supported. It has the same port stealing behaviour on all systems we support, but to achieve this it uses different syscalls on different platforms. macOS and other BSDs use SO_REUSEADDR, while Linux and Windows use SO_REUSEPORT. This behaviour matches libuv.

As a follow up PR we can add a reusePort option to enable the kernel connection load balancing behaviour found in Linux, for TCP sockets.

I think this PR is now good to go.

@lucacasonato lucacasonato changed the title feat(ext/net): added SO_REUSEADDR option for TCP sockets feat(ext/net): add reuseAddress option for UDP Oct 23, 2022
@Trolloldem
Copy link
Contributor Author

I have rebased this PR.

I have removed the reuseAddress option for TCP sockets. For TCP, this option is unconfigurably true on all platforms but Windows. This behaviour matches Node.js and Go. Neither of these systems support disabling SO_REUSEADDR on socket listeners.

For UDP, reuseAddress is supported. It has the same port stealing behaviour on all systems we support, but to achieve this it uses different syscalls on different platforms. macOS and other BSDs use SO_REUSEADDR, while Linux and Windows use SO_REUSEPORT. This behaviour matches libuv.

As a follow up PR we can add a reusePort option to enable the kernel connection load balancing behaviour found in Linux, for TCP sockets.

I think this PR is now good to go.

Thank you for reviewing my PR @lucacasonato! Just as a little remark, in your summary: macOS and other BSDs use SO_REUSEADDR, while Linux and Windows use SO_REUSEPORT. This behaviour matches libuv, it is actually the opposite (as pointed out in the comment referring libuv in ops.rs

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Co-authored-by: Trolloldem <gianluca.oldani@unibg.it>
@lucacasonato lucacasonato enabled auto-merge (squash) October 24, 2022 08:46
@lucacasonato lucacasonato merged commit 873a5ce into denoland:main Oct 24, 2022
@Trolloldem Trolloldem deleted the socket_options branch October 24, 2022 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants