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

Update naming around TcpListener::from_std so that it's harder to misuse #5597

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
82 changes: 76 additions & 6 deletions tokio/src/net/tcp/listener.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

We should fix it on all IO types if we're fixing it on one of them.

Copy link
Author

Choose a reason for hiding this comment

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

Aha the trap ^^
Does the wording seem good now? (Before I update them all following the same pattern.)

There's something that makes me feel a but uneasy however: it seems like the naming doesn't make as much sense as from_std did... from_tcp doesn't feel explanatory enough since there is TcpStream, TcpListener...

Now to fix that we could name it TcpStream::from_tcp_stream, however it is not as obvious in what it does as TcpFile::from_std.
With pipe there was only File so Pipe::from_file is explanatory enough. Also, the fact you're asserting that the file is a pipe is the main thing you're interested in in that context so it makes sense that it's named this way.
However with our function the main thing we're interested in about that function is that it goes from std to tokio.

=> How about naming it TcpFile::from_std_checked? That's more verbose, but at least it's more self-explanatory, you understand it right away when reading code that uses it, and it makes it pretty clear that it should be renamed to from_std in the next major release.

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right that it's a worse name than from_std. Your other suggestions could also work, but I don't think that they are clearly better than from_tcp.

Original file line number Diff line number Diff line change
Expand Up @@ -203,13 +203,54 @@ impl TcpListener {
///
/// # Notes
///
/// The caller is responsible for ensuring that the listener is in
/// non-blocking mode. Otherwise all I/O operations on the listener
/// This sets the socket to non-blocking mode if not already done, which
/// is necessary for normal operation within Tokio.
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
/// This sets the socket to non-blocking mode if not already done, which
/// is necessary for normal operation within Tokio.
/// This sets the socket to non-blocking mode if it isn't already non-blocking.

///
/// # Examples
///
/// ```rust,no_run
/// use std::error::Error;
/// use tokio::net::TcpListener;
///
/// #[tokio::main]
/// async fn main() -> Result<(), Box<dyn Error>> {
/// let std_listener = std::net::TcpListener::bind("127.0.0.1:0")?;
/// let listener = TcpListener::from_tcp(std_listener)?;
/// Ok(())
/// }
/// ```
///
/// # Panics
///
/// This function panics if it is not called from within a runtime with
/// IO enabled.
///
/// The runtime is usually set implicitly when this function is called
/// from a future driven by a tokio runtime, otherwise runtime can be set
/// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function.
#[track_caller]
pub fn from_tcp(listener: net::TcpListener) -> io::Result<TcpListener> {
listener.set_nonblocking(true)?;
Self::from_tcp_unchecked(listener)
}

/// Creates new `TcpListener` from a `std::net::TcpListener` without
/// checking that it's non-blocking.
///
/// This function is intended to be used to wrap a TCP listener from the
/// standard library in the Tokio equivalent. However, it does not check
/// that it's already in non-blocking mode.
///
/// Instead, the caller is responsible for ensuring that the listener is in
/// non-blocking mode, otherwise all I/O operations on the listener
/// will block the thread, which will cause unexpected behavior.
/// Non-blocking mode can be set using [`set_nonblocking`].
///
/// [`set_nonblocking`]: std::net::TcpListener::set_nonblocking
///
/// You may generally prefer using [`from_tcp`](TcpListener::from_tcp),
/// which does that for you if not already done.
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
/// You may generally prefer using [`from_tcp`](TcpListener::from_tcp),
/// which does that for you if not already done.
/// You should usually prefer to use [`from_tcp`](TcpListener::from_tcp).

Copy link
Member

Choose a reason for hiding this comment

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

Small nit: for reference documentation, I think it's better to avoid "you". I'd rewrite this as "It may be preferable to use from_tcp, which sets set_nonblocking."

///
/// # Examples
///
/// ```rust,no_run
Expand All @@ -220,7 +261,7 @@ impl TcpListener {
/// async fn main() -> Result<(), Box<dyn Error>> {
/// let std_listener = std::net::TcpListener::bind("127.0.0.1:0")?;
/// std_listener.set_nonblocking(true)?;
/// let listener = TcpListener::from_std(std_listener)?;
/// let listener = TcpListener::from_tcp_unchecked(std_listener)?;
/// Ok(())
/// }
/// ```
Expand All @@ -234,12 +275,29 @@ impl TcpListener {
/// from a future driven by a tokio runtime, otherwise runtime can be set
/// explicitly with [`Runtime::enter`](crate::runtime::Runtime::enter) function.
#[track_caller]
pub fn from_std(listener: net::TcpListener) -> io::Result<TcpListener> {
pub fn from_tcp_unchecked(listener: net::TcpListener) -> io::Result<TcpListener> {
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation of this method should have a bigger focus on the fact that this is an unchecked conversion. Please see from_file_unchecked for inspiration.

Copy link
Member

Choose a reason for hiding this comment

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

Is the the fact that the underlying file descriptor might not be set to non-blocking the only behavioral change? Even if so, this seems like reasonable naming.

(I'm assuming that's the naming feedback you're looking for.)

Copy link
Contributor

Choose a reason for hiding this comment

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

In my original suggestion it would also return an error if the underlying fd is not actually a tcp socket, but we can debate whether we want that.

let io = mio::net::TcpListener::from_std(listener);
let io = PollEvented::new(io)?;
Ok(TcpListener { io })
}

/// Creates new `TcpListener` from a `std::net::TcpListener`.
///
/// This function is deprecated because it's easy to misuse,
/// and naming doesn't warn enough about it.
///
/// You should typically favor using
/// [`from_tcp`](TcpListener::from_tcp) instead of
/// [`from_tcp_unchecked`](TcpListener::from_tcp_unchecked.
///
/// This function however has the same behavior as
/// [`TcpListener::from_tcp_unchecked`].
#[track_caller]
#[deprecated = "Easy to misuse - use from_tcp or from_tcp_unchecked instead"]
pub fn from_std(listener: net::TcpListener) -> io::Result<TcpListener> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Another option is to just #[doc(hidden)] this.

Copy link
Author

@Ten0 Ten0 Apr 5, 2023

Choose a reason for hiding this comment

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

Ah yes, didn't think about that, that avoids cluttering the doc with deprecated stuff :)

Self::from_tcp_unchecked(listener)
}

/// Turns a [`tokio::net::TcpListener`] into a [`std::net::TcpListener`].
///
/// The returned [`std::net::TcpListener`] will have nonblocking mode set as
Expand Down Expand Up @@ -384,9 +442,21 @@ impl TryFrom<net::TcpListener> for TcpListener {
/// Consumes stream, returning the tokio I/O object.
///
/// This is equivalent to
/// [`TcpListener::from_std(stream)`](TcpListener::from_std).
/// [`TcpListener::from_tcp_unchecked(stream)`](TcpListener::from_tcp_unchecked).
///
/// # Notes
///
/// The caller is responsible for ensuring that the listener is in
/// non-blocking mode. Otherwise all I/O operations on the listener
/// will block the thread, which will cause unexpected behavior.
/// Non-blocking mode can be set using [`set_nonblocking`].
///
/// [`set_nonblocking`]: std::net::TcpListener::set_nonblocking
///
/// You may generally prefer using [`TcpListener::from_tcp`],
/// which does that for you if not already done.
fn try_from(stream: net::TcpListener) -> Result<Self, Self::Error> {
Self::from_std(stream)
Self::from_tcp_unchecked(stream)
}
}

Expand Down
2 changes: 1 addition & 1 deletion tokio/tests/io_driver_drop.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests seem to be related to runtime shutdown. Not sure what the idea with tcp_doesnt_block was.

Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ fn tcp_doesnt_block() {
let listener = {
let _enter = rt.enter();
let listener = std::net::TcpListener::bind("127.0.0.1:0").unwrap();
TcpListener::from_std(listener).unwrap()
TcpListener::from_std(listener).unwrap() // Given the name of this test it looks like it hasn't fulfilled its purpose when going from mio 0.6 to mio 0.7 -> TODO investigate why
};

drop(rt);
Expand Down
2 changes: 1 addition & 1 deletion tokio/tests/net_panic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ fn tcp_listener_from_std_panic_caller() -> Result<(), Box<dyn Error>> {
let panic_location_file = test_panic(|| {
let rt = runtime_without_io();
rt.block_on(async {
let _ = TcpListener::from_std(std_listener);
let _ = TcpListener::from_tcp_unchecked(std_listener);
});
});

Expand Down
4 changes: 3 additions & 1 deletion tokio/tests/no_rt.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,5 +37,7 @@ async fn timeout_value() {
expected = "there is no reactor running, must be called from the context of a Tokio 1.x runtime"
)]
fn io_panics_when_no_tokio_context() {
let _ = tokio::net::TcpListener::from_std(std::net::TcpListener::bind("127.0.0.1:0").unwrap());
let _ = tokio::net::TcpListener::from_tcp_unchecked(
std::net::TcpListener::bind("127.0.0.1:0").unwrap(),
);
}