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

Ten0 · 2023-04-03T19:23:24Z

Resolves #5595

Motivation

Documentation over at

Lines 206 to 209 in 88445e7

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

is easy to miss.

Solution

Providing that information in a more obvious way through the naming reduces the risk for users to spend hours tracking down nasty deadlock bugs.

TODO before merge:
Figure out the correct way to update
https://github.com/Ten0/tokio/blob/4262749c466aa55851153c7e0a6e3b992bebef37/tokio/tests/io_driver_drop.rs#L15
and
https://github.com/Ten0/tokio/blob/4262749c466aa55851153c7e0a6e3b992bebef37/tokio/tests/io_driver_drop.rs#L34

The first one seems (from the naming but I may be mistaken here) like it was meant to prevent us from what happened with that function (which originally did the check and stopped doing it as we updated mio but didn't super-notice it and did not update the doc), but it failed to deadlock as expected, so we probably want it to now test the from_tcp function and make it fail on from_tcp_unchecked.

The second one seems to follow a similar pattern so it's probably best to fix it with all the elements from the first one in mind..

Darksonn · 2023-04-03T20:00:10Z

tokio/src/net/tcp/listener.rs

@@ -234,12 +281,26 @@ 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> {


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.

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

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.

tokio/src/net/tcp/listener.rs

Darksonn · 2023-04-03T20:04:32Z

tokio/tests/io_driver_drop.rs

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

Darksonn · 2023-04-03T20:05:12Z

tokio/src/net/tcp/listener.rs

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

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.

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.

Darksonn · 2023-04-05T15:16:36Z

tokio/src/net/tcp/listener.rs

+    /// This sets the socket to non-blocking mode if not already done, which
+    /// is necessary for normal operation within Tokio.


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.

Darksonn · 2023-04-05T15:17:59Z

tokio/src/net/tcp/listener.rs

+    /// You may generally prefer using [`from_tcp`](TcpListener::from_tcp),
+    /// which does that for you if not already done.


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

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

Darksonn · 2023-04-05T15:18:33Z

tokio/src/net/tcp/listener.rs

+    /// 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> {


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

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

Darksonn · 2023-04-05T16:05:30Z

We discussed the naming issue on discord, and we decided that we would like to solve this issue via a different approach instead.

For now, we suggest adding a check to from_std that checks whether the socket is non-blocking, and prints a warning if it isn't. (The check would only be used if debug assertions are enabled.) Later, we could potentially replace it with a panic and add an from_std_unchecked that doesn't panic.

Ten0 · 2023-04-05T21:07:30Z

We discussed the naming issue on discord

For reference:
https://discord.com/channels/500028886025895936/500336346770964480/1093193069408567386

Ten0 · 2023-04-05T21:07:42Z

We can't change the existing method

We should not have a std conversion that sets non blocking

Since you all seem to agree on this and I'm not attached to it being changed under the hood either (even though I think it's ultimately a better public interface), the below reasoning assumes it's not acceptable to update the behavior like savages and pretend it's backwards-compatible.

Now comparing the potential solutions that have been mentioned so far for that scenario:

Add a warning if people call from_std with a channel that is not set to non_blocking
- Does not require any update from the users as it is released
- Can slip through review and CI, turning into a runtime bug instead of being simply noticed at compile-time. It's still very non-obvious for users that they should take care of this as they write the code, and people will regularly hit that issue and spend some time fixing it. If they don't notice it because e.g. it does not cause issues when they test with a single client, that could cause severe issues in production.
- Can generate gigabytes of cloud logs that end up costing thousands of euros (yeah that was a not-so-cool not-so-surprise)
Eventually panic
- Can slip through review and CI, turning into a runtime crash. It's still very non-obvious for users that they should take care of this as they write the code, and people will regularly hit that panic and spend some time fixing it.
- Can stop existing apps that work by chance despite not having set non_blocking from working. That doesn't seem ok right away in a semver-compatible release.
Deprecate the current method, make the public method that users would most naturally use to convert from std set non-blocking, and make the way to construct without ensuring it is set in any way very explicit on the requirement, as part of the function name
- Covers for the issue at compile time
- Simplifies most calling code, e.g. https://github.com/hyperium/hyper/blob/a9d4e8321c141d416cdbddb1effe093aad7c7726/src/server/tcp.rs#L125-L132
- However, it requires anyone who used the newly deprecated function to update to the new construct
  - Since new writing is likely either better (cf just above) or correct when the previous one wasn't, it's probably not that bad
- from_std is already the ideal naming, and it's taken by the function we want to deprecate, so we need to find another one.
  - from_tcp does not work (Update naming around TcpListener::from_std so that it's harder to misuse #5597 (comment), https://discord.com/channels/500028886025895936/1093193069408567386/1093195379299860652)
  - We haven't agreed on a good naming for the variant that does not set non-blocking either. from_std_unchecked is not semantically correct. (https://discord.com/channels/500028886025895936/1093193069408567386/1093202196381827203)
  - How about from_std_set_nonblocking, and from_std_assume_nonblocking? (The former can then be turned into from_std in the next major release as it seems that is what people will most likely want to use)
    - It was mentioned on Discord "i would prefer adding "unchecked" variants on the socket builder path instead of Tcp{Stream,Listener} directly" -> Since most people shouldn't use this I would have favored that as well, however I can't find where these are. Could you point me to the relevant location ?

To my eyes it looks like the benefits of solution 3 on the long term clearly outweigh its single downside (if it's even one since resulting code would typically be more straightforward). I didn't understand from reading the previous discussions the arguments that make 1/2 better than 3.

We should not have a std conversion that sets non blocking

What did I miss?
Thanks

PS: The only two cases I see where it wouldn't be acceptable to make from_std turn stuff into non-blocking are:

if it's an expensive system call that some people currently rely on not being made several times
if some people rely on constructing this without setting it to non-blocking, and then turning it back into the STD counterpart using into_std without having to set it back to blocking

Are there any other scenarios? Which is the one that makes it unacceptable?

carllerche · 2023-04-06T17:06:57Z

There needs to be a pass-through conversion method from std types -> Tokio types. The intent of from_std is you call it after taking care of the necessary configuration, and Tokio doesn''t modifies the configuration anymore.

The current proposal is to:

First, add a warning when debug assertions are enabled (dev/test mode mostly).
Eventually, switch the warning to a debug assertion, i.e., a check and panic when debug assertions are enabled. Again this would only happen in dev/test mode.

Can slip through review and CI, turning into a runtime bug.

When this is a panic, it would only slip in if the code path is not tested in CI.

Can generate gigabytes of cloud logs

Again, this would be one log line per socket passed to from_std when debug assertions are enabled. This will only result in a significant amount of logs if several other errors have been made along the way. Additionally, the quantity of log entries generated would be limited by the code running poorly in the first place (not compiled with --release and libraries running expensive debug assertion logic).

Can slip through review and CI, turning into a runtime crash.

Assuming you mean in release, there would be no crash because the panic would not be enabled.

Can stop existing apps that work by chance despite not having set non_blocking from working. That doesn't seem ok right away in a server-compatible release.

Those apps would still work the same when compiled with --release, additionally, it would only change behavior when calling the method incorrectly. This is no different than any bug fix that changes incorrect logic when misusing an API. We have done this in the past. We also would release this as a minor release. Any users expecting greater stability should be tracking LTS branches.

Deprecate the current method, make the public method that users would most naturally use to convert from std set non-blocking, and make the way to construct without ensuring it is set in any way very explicit on the requirement, as part of the function name

This does not entirely solve the issue. We also implement TryFrom<std::net::TcpStream> for TcpStream. These trait implementations call from_std and trait implementation cannot be deprecated.

That said, for the record, if we were able to find a consistent way to deprecate from_std and the trait implementation and we end up going down that path, I believe the best names are:

from_std_unchecked
from_blocking, from_std_blocking, or from_blocking_std (not sure which permutation).

Ten0 · 2023-04-06T17:27:18Z

when debug assertions are enabled

Ah I missed "debug mode". That seems to solve most points indeed.

[Deprecating the current method] does not entirely solve the issue. We also implement TryFromstd::net::TcpStream for TcpStream. These trait implementations call from_std and trait implementation cannot be deprecated.

So you think doing both would be too much dedicated code (bloat) for the relative gain between both solutions?

carllerche · 2023-04-06T19:22:37Z

If we could deprecate the trait implementation, I would be more favorable to deprecating from_std. In this case, consistency wins out for me (the conversion TryFrom implementation exists as a pair to from_std). By keeping from_std we also have a method with documentation that can highlight the risk. If we deprecate that method, the TryFrom implementation will still be available but without visible documentation.

…ner_from_std

Ten0 · 2023-08-06T13:07:25Z

Plot twist: there is no is_blocking/is_nonblocking function on any of the corresponding std objects: https://doc.rust-lang.org/stable/std/net/index.html?search=blocking

So I'm not sure how to detect whether they are configured as blocking.

If that prevents the runtime warning approach, I feel like deprecating from_std and just being sad that ATM we can't do anything about the TryFrom implementation is still better than doing nothing.

Darksonn · 2023-08-06T15:12:15Z

You can query it using socket2::SockRef::nonblocking.

Ten0 · 2023-08-06T18:09:34Z

You can query it using socket2::SockRef::nonblocking.

Wonderful. That's done.

I think I'm reasonably happy with the current state with regards to TcpListener.
I'd like this one to be fully approved before I go and do the exact same thing on all the others.

Just a few points I'd like to highlight for discussion so that they are not missed during the review:

By keeping from_std we also have a method with documentation that can highlight the risk. If we deprecate that method, the TryFrom implementation will still be available but without visible documentation.

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

I think you guys need to align on whether this ends up being doc hidden or not.

(Again AFAICT just deprecating the method instead of hiding it serves both the purpose of documentation (hinting that this used to be the default behavior so likely the one used in the TryFrom impl) and of encouraging people to write the clear version of the code (because that would be impossible to miss). If I were to maintain a crate using from_std, this would definitely be what I would want to see:)
With regards to the naming for the time being I went with from_std_set_nonblocking and from_std_assume_nonblocking. This is slightly more verbose than the other alternatives proposed, but homogeneous and very clear. We're all using autocomplete anyway. Pease have a look at how it feels:

Downsides of the other namings proposed so far compared to these:
- from_tcp -> The semantic information required is that it's from the std type. User will already know it's TCP.
- from_std_blocking -> xxx_blocking typically means the blocking version of an otherwise async method. One might think that this method might block, and that they should use from_std instead as they are in a non-blocking environment.
- from_blocking_std
  - Doesn't show as easily when searching from_std, which is the natural name to search for. (In particular it won't show next to from_std_unchecked in the list):
  - It's actually not a requirement that it's currently set to blocking when calling this function. You may very well call it if you don't know that it's blocking. (I don't think this ever has a significant cost, does it?) The name seems to imply otherwise.
- from_std_unchecked -> It's not necessarily clear to users what is unchecked. For instance one might imagine it's "I won't check that this Fd represents a TcpListener", and think "but I know it's a TcpListener so this is fine".
I've put the check in case debug assertions are enabled on from_std_assume_nonblocking, not only on from_std. I'm assuming somebody calling this mistakenly would be happy to see the warning as well. This is not ok if there is any legitimate use-case for constructing a tokio TcpListener with a blocking socket inside.
There's still this test I'm not sure what to do with:
https://github.com/Ten0/tokio/blob/67078dde7e194cef031bf5d58756a63abaa9049b/tokio/tests/io_driver_drop.rs#L15

Thanks!

Darksonn · 2023-08-09T14:14:46Z

Hmm. I like the names from_std_set_nonblocking and from_std_assume_nonblocking.

If we're deprecating the method, then I don't think it's necessary to use #[doc(hidden)] or the warning. But I would like to get Carl's input as well.

carllerche · 2023-08-09T20:03:24Z

If we deprecate, I would doc hide it.

I don't love the names. What about:

from_blocking_std
from_nonblocking_std

hawkw · 2023-08-09T21:55:30Z

If we deprecate, I would doc hide it.

I don't love the names. What about:

from_blocking_std

from_nonblocking_std

I might prefer from_std_blocking/from_std_nonblocking, because from_{blocking, nonblocking}_std won't show up in searches for the string "from_std", while from_std_{blocking, nonblocking} do contain "from_std" as a substring.

Sorry for adding additional alternatives to bikeshed over! :P

Ten0 · 2023-08-09T21:58:58Z

What about from_blocking_std and from_nonblocking_std

I agree it's much better than from_blocking_std/from_std_unchecked.
However, although first point is less impactful because these would be homogeneous, I slightly prefer assume/set for the reasons mentioned about specifically from_blocking_std in my previous message.
Any particular other reason or it's just the bigger length?

Ten0 · 2023-08-09T22:11:00Z

because from_{blocking, nonblocking}_std won't show up in searches for the string "from_std", while from_std_{blocking, nonblocking} do contain "from_std" as a substring.

for the reasons mentioned about specifically from_blocking_std in my previous message

Sounds a lot like the reasons I gave in that summary.

from_std_blocking - Sorry for adding additional alternatives to bikeshed over! :P

Also already analyzed in that same message why I think specifically from_std_blocking may be confusing to some users.

AFAICT from_std_set/assume_nonblocking is the only one suggested so far that hasn't got the already mentioned downsides. Any new element to add to the weighing? Or is it just that from_std_set_nonblocking and from_std_assume_nonblocking are too long?

Are my messages too long? 😕

Darksonn · 2023-08-10T08:24:18Z

I can't think of any downsides other than length. They're much much more clear about what they do than from_std_blocking/ from_std_nonblocking.

Are my messages too long?

No, I think they're fine. I think the summary from 4 days ago ways very useful.

I've put the check in case debug assertions are enabled on from_std_assume_nonblocking, not only on from_std. I'm assuming somebody calling this mistakenly would be happy to see the warning as well. This is not ok if there is any legitimate use-case for constructing a tokio TcpListener with a blocking socket inside.

I think I probably prefer to leave the behavior of from_std unchanged, and have from_std_assume_nonblocking panic in debug mode. I think there's value in not changing behavior of existing functions for backwards compatibility. There could be someone who first calls from_std and makes it nonblocking afterwards.

Ten0 · 2023-08-10T10:14:39Z

I probably prefer to leave the behavior of from_std unchanged

Unchanged as in "as currently implemented in this PR" or as in "as currently implemented on master, but deprecated"? (The whole point of this feature being to prevent the debugging nightmares caused by using from_tcp and accidentally passing something blocking)

Darksonn · 2023-08-10T15:09:15Z

As in, unchanged from master. I think deprecating (this triggers warnings) and adding alternatives is probably enough to get people to stop using it.

Ten0 · 2023-08-10T18:10:22Z

As in, unchanged from master.

So it looks like you disagree with Carl that consistency with the TryFrom impl wins. (#5597 (comment))

I agree with you but I don't think that weighs too much. Looks like you guys need to align 😉

wip

4262749

Darksonn added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Apr 3, 2023

Darksonn reviewed Apr 3, 2023

View reviewed changes

improve wording

05dc864

Darksonn reviewed Apr 5, 2023

View reviewed changes

Ten0 added 2 commits August 6, 2023 14:38

Merge remote-tracking branch 'upstream/master' into 5595_better_liste…

5c19066

…ner_from_std

comments and naming improvements

9e99027

Ten0 added 3 commits August 6, 2023 19:01

finish for TcpListener

b80291d

actually put the check in from_std_unchecked

37c5af5

make it explicit with regards to what is unchecked

67078dd

Darksonn mentioned this pull request Feb 3, 2024

Add checks in from_std to ensure that it is non-blocking #3539

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Ten0 commented Apr 3, 2023

Darksonn Apr 3, 2023

davidbarsky Apr 5, 2023 •

edited

Darksonn Apr 5, 2023

Darksonn Apr 3, 2023

Darksonn Apr 3, 2023

Ten0 Apr 5, 2023

Darksonn Apr 5, 2023

Darksonn Apr 5, 2023

Darksonn Apr 5, 2023

davidbarsky Apr 5, 2023

Darksonn Apr 5, 2023

Ten0 Apr 5, 2023 •

edited

Darksonn commented Apr 5, 2023

Ten0 commented Apr 5, 2023

Ten0 commented Apr 5, 2023 •

edited

carllerche commented Apr 6, 2023

Ten0 commented Apr 6, 2023

carllerche commented Apr 6, 2023

Ten0 commented Aug 6, 2023

Darksonn commented Aug 6, 2023

Ten0 commented Aug 6, 2023

Darksonn commented Aug 9, 2023

carllerche commented Aug 9, 2023

hawkw commented Aug 9, 2023

Ten0 commented Aug 9, 2023

Ten0 commented Aug 9, 2023

Darksonn commented Aug 10, 2023

Ten0 commented Aug 10, 2023 •

edited

Darksonn commented Aug 10, 2023

Ten0 commented Aug 10, 2023 •

edited

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

		/// This sets the socket to non-blocking mode if not already done, which
		/// is necessary for normal operation within Tokio.

		/// You may generally prefer using [`from_tcp`](TcpListener::from_tcp),
		/// which does that for you if not already done.

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

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

Are you sure you want to change the base?

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

Conversation

Ten0 commented Apr 3, 2023

Motivation

Solution

Choose a reason for hiding this comment

davidbarsky Apr 5, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ten0 Apr 5, 2023 • edited

Choose a reason for hiding this comment

Darksonn commented Apr 5, 2023

Ten0 commented Apr 5, 2023

Ten0 commented Apr 5, 2023 • edited

carllerche commented Apr 6, 2023

Ten0 commented Apr 6, 2023

carllerche commented Apr 6, 2023

Ten0 commented Aug 6, 2023

Darksonn commented Aug 6, 2023

Ten0 commented Aug 6, 2023

Darksonn commented Aug 9, 2023

carllerche commented Aug 9, 2023

hawkw commented Aug 9, 2023

Ten0 commented Aug 9, 2023

Ten0 commented Aug 9, 2023

Darksonn commented Aug 10, 2023

Ten0 commented Aug 10, 2023 • edited

Darksonn commented Aug 10, 2023

Ten0 commented Aug 10, 2023 • edited

davidbarsky Apr 5, 2023 •

edited

Ten0 Apr 5, 2023 •

edited

Ten0 commented Apr 5, 2023 •

edited

Ten0 commented Aug 10, 2023 •

edited

Ten0 commented Aug 10, 2023 •

edited