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.

Sign up for GitHub

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

Jump to bottom

Tcp socket from std stream #3838

Merged
merged 6 commits into from Jun 11, 2021
Merged

Tcp socket from std stream #3838

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
28eb241
convert std TcpStream into TcpSocket
AtulJatia Jun 5, 2021
bab7c2c
convert std TcpStream into TcpSocket
AtulJatia Jun 5, 2021
01e54bb
Merge branch 'tcp-socket-from-std-stream' of https://github.com/AtulJ…
AtulJatia Jun 5, 2021
309036d
Review changes
AtulJatia Jun 8, 2021
629876a
Review changes
AtulJatia Jun 10, 2021
8057f89
Fixed documentation tests
AtulJatia Jun 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 42 additions & 0 deletions tokio/src/net/tcp/socket.rs
View file
Open in desktop
Expand Up @@ -482,6 +482,48 @@ impl TcpSocket {
let mio = self.inner.listen(backlog)?;
TcpListener::new(mio)
}

/// 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.

/// # Examples
/// ```
/// use tokio::net::TcpSocket;
///
/// 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<()> {

///
/// let stream = TcpStream::connect("127.0.0.1:8080")?;
///
/// 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);

///
/// 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.

/// }
/// ```
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 {

#[cfg(unix)]
{
use std::os::unix::io::{FromRawFd, IntoRawFd};

let raw_fd = std_stream.into_raw_fd();
unsafe { Ok(TcpSocket::from_raw_fd(raw_fd)) }
}

#[cfg(windows)]
{
use std::os::windows::io::{FromRawSocket, IntoRawSocket};

let raw_socket = std_stream.into_raw_socket();
unsafe { Ok(TcpSocket::from_raw_socket(raw_socket)) }
}
}
}

impl fmt::Debug for TcpSocket {
Expand Down