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): Use Socket2 crate to create TcpListener #13808

Merged
merged 2 commits into from
Mar 4, 2022
Merged

feat(ext/net): Use Socket2 crate to create TcpListener #13808

merged 2 commits into from
Mar 4, 2022

Conversation

Trolloldem
Copy link
Contributor

@Trolloldem Trolloldem commented Mar 1, 2022

After a first attempt in solving #1195 in #13794, it has been noticed that the SO_REUSEADDR option has to be defaulted to true in order to pass the net unit tests. In the same PR was proposed to do the porting to Socket2 and the support to the options SO_REUSEADDR and SO_REUSEPORT in two separate PR. This PR presents the TcpListener implementation based on Socket2.

It is possible to notice that:

  1. the SO_REUSEADDR option is defaulted to true only for non-windows systems, following the implementation of TcpListener.bind() of std::net
  2. similarly, the socket type is set to SOCK_STREAM and the backlog argument of listen() is set to 128

These steps should produce a TcpListener equal to the one produce by the instruction std::net::TcpListener::bind(&addr)?;

@bartlomieju
Copy link
Member

@Trolloldem something is off in this PR, it contains commits that were already landed on main branch

@Trolloldem
Copy link
Contributor Author

@Trolloldem something is off in this PR, it contains commits that were already landed on main branch

My bad, now it should be solved with a correct rebase

@Trolloldem
Copy link
Contributor Author

@bartlomieju the CI failure seems related to a rustc failure. On my branch all the CI seem to pass. It is possible to try to re-run the tests?

@bartlomieju
Copy link
Member

@bartlomieju the CI failure seems related to a rustc failure. On my branch all the CI seem to pass. It is possible to try to re-run the tests?

Yes, you need to push empty commit to trigger new CI run: git commit --allow-empty -m "reset CI"

@Trolloldem
Copy link
Contributor Author

@bartlomieju all CI seem fine now. Are there any additional changes / information required?

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.

LGTM, thank you @Trolloldem

@bartlomieju bartlomieju merged commit 7e34964 into denoland:main Mar 4, 2022
@Trolloldem Trolloldem deleted the socket2 branch March 5, 2022 10:31
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.

2 participants