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

Add TcpSocket, a basic TCP socket builder #1358

Merged
merged 10 commits into from
Oct 6, 2020
Merged

Add TcpSocket, a basic TCP socket builder #1358

merged 10 commits into from
Oct 6, 2020

Conversation

carllerche
Copy link
Member

@carllerche carllerche commented Oct 2, 2020

@Thomasdezeeuw I'm posting this now for early feedback. The work is not done.

This provides TcpSocket, a basic API for building a TCP socket. The goal is not to provide comprehensive coverage of all system options, but to provide an API for the most common cases.

This is added now as a replacement for the removal of TcpStream::connect_std. The connect_std function from v0.6 was used until now as the strategy to set socket option before obtaining a mio TcpStream.

Providing some strategy for customizing a TcpStream is required for Hyper to be able to upgrade.

The TcpSocket API for Tokio is provided here. There has been some discussion in Discord related to how to handle configuring a TCP socket.

I know there is some discussion about using socket2 for advanced socket configuration. I don't believe TcpSocket and socket2 are mutually exclusive. TcpSocket provides the ability to configure a socket while maintaining mio invariants (non-blocking).

This provides `TcpSocket`, a basic API for building a TCP socket. The
goal is not to provide comprehensive coverage of all system options, but
to provide an API for the most common cases.

This is added now as a replacement for the removal of
`TcpStream::connect_std`. The `connect_std` function from v0.6 was used
until now as the strategy to set socket option before obtaining a mio
TcpStream.

Providing some strategy for customizing a `TcpStream` is required for
Hyper to be able to upgrade.
@carllerche
Copy link
Member Author

@Thomasdezeeuw This is ready to go!

Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Overall I still think something like this is better in a crate like socket2. However since that is still a way away I'm OK approving this.

For the review, I just have two small things.

})
}

pub(crate) fn new_for_addr(addr: SocketAddr) -> io::Result<TcpSocket> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not public? Perhaps with a new name: TcpSocket::for_address.

Copy link
Member Author

Choose a reason for hiding this comment

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

Mostly to minimize the API surface initially, but I can make it public.

src/sys/windows/tcp.rs Outdated Show resolved Hide resolved
@carllerche carllerche merged commit 5b09e60 into master Oct 6, 2020
@Thomasdezeeuw Thomasdezeeuw deleted the tcp-socket branch June 19, 2022 16:51
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