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

Tcp socket from std stream #3838

Merged
merged 6 commits into from Jun 11, 2021

Conversation

AtulJatia
Copy link
Contributor

convert std TcpStream into TcpSocket

This converts the TcpStream into the platform specific raw_fd(unix) / raw_socket(windows)
and then converts them into the TcpSocket as TcpSocket implements the
FromRawFd(unix) and FromRawSocket(windows) traits.

Fixes: #3734

This converts the TcpStream into the platform
specific raw_fd(unix) / raw_socket(windows)
and then converts them into the TcpSocket
as TcpSocket implements the FromRawFd(unix) and
FromRawSocket(windows) traits.

Fixes: tokio-rs#3734
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

Comment on lines 486 to 492
/// Converts a TcpStream into a TcpSocket
/// This converts the TcpStream into
/// the platform specific raw_fd(unix) / raw_socket(windows)
/// and then converts them into the TcpSocket
/// as TcpSocket implements the FromRawFd(unix) and
/// FromRawSocket(windows) traits.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have some improvements to the documentation:

  • We typically wrap the text such that it goes all the way out to 80 characters wide. If you want to make a newline that shows up in the documentation, you need an empty line, as it otherwise just concatenates the lines.
  • I would like TcpStream to be a link, and TcpSocket should be surrounded by backticks to display it as code.
  • You do not have to explain how it does the conversion.
  • The documentation should make it clear that this is intended for usage together with socket2, and that the socket must not be in a connected state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your quick reply. I'll go over all the changes suggested and raise another request.

Copy link
Contributor

Choose a reason for hiding this comment

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

You should not create a new PR. Just push more commits to your tcp-socket-from-std-stream branch.

Comment on lines 500 to 507
/// async fn main() -> io::Result<()> {
///
/// let stream = TcpStream::connect("127.0.0.1:8080")?;
///
/// let socket = TcpSocket::from_std_stream(stream)?;
/// # drop(socket);
///
/// Ok(())
Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to use socket2 in this example. Your example is currently converting a socket that is in a connected state, which you aren't supposed to do.

/// Ok(())
/// }
/// ```
pub fn from_std_stream(std_stream: std::net::TcpStream) -> io::Result<TcpSocket> {
Copy link
Contributor

Choose a reason for hiding this comment

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

The conversion always succeeds, so no need for a Result.

Suggested change
pub fn from_std_stream(std_stream: std::net::TcpStream) -> io::Result<TcpSocket> {
pub fn from_std_stream(std_stream: std::net::TcpStream) -> TcpSocket {

Comment on lines 501 to 503
/// let stream = TcpStream::from(socket2_socket);
///
/// let socket = TcpSocket::from_std_stream(stream)?;
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer simplifying it a bit to this.

Suggested change
/// let stream = TcpStream::from(socket2_socket);
///
/// let socket = TcpSocket::from_std_stream(stream)?;
/// let socket = TcpSocket::from_std_stream(socket2_socket.into())?;

/// let stream = TcpStream::from(socket2_socket);
///
/// let socket = TcpSocket::from_std_stream(stream)?;
/// # drop(socket);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't do anything.

Suggested change
/// # drop(socket);

Comment on lines 486 to 487
/// Converts a `socket2::Socket` into a `TcpSocket`. The socket2 socket
/// needs to be in a disconnected state when passed as an argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Converts a `socket2::Socket` into a `TcpSocket`. The socket2 socket
/// needs to be in a disconnected state when passed as an argument.
/// Converts a [`std::net::TcpStream`] into a `TcpSocket`. The provided
/// socket must not have been connected prior to calling this function. This
/// function is typically used together with crates such as [`socket2`] to
/// configure socket options that are not available on `TcpSocket`.
///
/// [`std::net::TcpStream`]: struct@std::net::TcpStream
/// [`socket2`]: https://docs.rs/socket2/

/// use std::net::TcpStream;
///
/// #[tokio::main]
/// async fn main() -> io::Result<()> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Your example doesn't compile.

Suggested change
/// async fn main() -> io::Result<()> {
/// async fn main() -> std::io::Result<()> {

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Jun 11, 2021
@Darksonn
Copy link
Contributor

Thanks for doing this!

@Darksonn Darksonn merged commit 2e44cd2 into tokio-rs:master Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate M-net Module: tokio/net
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TcpSocket::from_std
2 participants