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

[Ten0]

trafficstars

Resolves #5595

Motivation

Documentation over at https://github.com/tokio-rs/tokio/blob/88445e762c569b538ac3f8d3dbb76887db421207/tokio/src/net/tcp/listener.rs#L206-L209 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]

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]

We discussed the naming issue on discord

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

[Ten0]

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:

1. 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)
2. 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.
3. 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 (https://github.com/tokio-rs/tokio/pull/5597#discussion_r1158403427, 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]

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]

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]

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.

[Ten0]

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]

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

[Ten0]

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:

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

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

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

4. 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]

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]

If we deprecate, I would doc hide it.

I don't love the names. What about:

• from_blocking_std
• from_nonblocking_std

[hawkw]

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]

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]

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]

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]

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]

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]

As in, unchanged from master.

So it looks like you disagree with Carl that consistency with the TryFrom impl wins. (https://github.com/tokio-rs/tokio/pull/5597#issuecomment-1499512782)

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