-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Conversation
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? |
@Trolloldem you are hitting an internal compiler error in Rust. You can (most likely) fix this, by changing cache keys in |
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:
The tests |
There was a problem hiding this 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.
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
@@ -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. */ |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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 To enable this option, the Socket2 crate has been used, using the same settings of the std::net implementation of Similarly to node (win udp, unix udp), the options always defaults to |
@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? |
@littledivy @lucacasonato could you take a look at this PR? |
I have rebased this PR. I have removed the For UDP, As a follow up PR we can add a I think this PR is now good to go. |
Thank you for reviewing my PR @lucacasonato! Just as a little remark, in your summary: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this 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>
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 withSO_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.