TcpListener::from_std is super easy to misuse

[Ten0]

Is your feature request related to a problem? Please describe.
I lost about 6 hours tracking down a deadlock bug that ended up leading me to this in TcpListener::from_std:

tokio/tokio/src/net/tcp/listener.rs

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

I would argue that this API is very prone to misuse, especially since going through it is the only way to attempt binding an address outside of tokio then turn it into a tokio-usable one (even in the context of hyper/axum).

Describe the solution you'd like
I'd like the public API for this feature of converting from STD to tokio to make the necessary std_listener.set_nonblocking(true) call.

Alternately, I think if we have a function that does not make that call, it should be named in a way that makes it explicit that the user has to do that specific thing that's not enforced by typing before calling it, e.g. TcpListener::from_std_manually_configured_as_non_blocking

Additional context
It seems that this used to be the way this worked, but was changed propagating a change from downstream.
I would tend to think that it may have been best to not propagate that change directly, and instead do one of the two things above.

[Darksonn]

Yes, I agree, this has been a problem for a while. We can't change the existing method, but maybe we could deprecate it and replace it with from_tcp and from_tcp_unchecked where the first method sets is as non-blocking and checks that it's actually a tcp stream. This would be similar to from_file on pipes.

[Ten0]

maybe we could deprecate it and replace it with from_tcp and from_tcp_unchecked where the first method sets is as non-blocking and checks that it's actually a tcp stream. This would be similar to from_file on pipes.

That sounds like a great idea. Should I open a PR?

[Ten0]

I'm not sure what you mean by "checks that it's actually a tcp stream" though: the entry type is TcpListener so AFAICT that in particular is already guaranteed by typing, no?

[Darksonn]

Ah, well, we have that check on the fifo pipe method I mentioned.

[Ten0]

That seems to be checking that the file is a fifo, which seems to not be guaranteed by typing there.

(Just to make sure it wasn't missed: Should I open a PR?)

[Darksonn]

Yes, go ahead and open a PR.

[tsar-boomba]

Just fixed a bug from 2 years ago caused by this. Would love to get this merged in ASAP. Will do anything to help out.

[Ten0]

Hello!
IIRC the draft PR is pending an answer to point 4 of this comment as well as to this comment.

[rupert648]

I was just caught out by this as well 😅
Is there anything we can do to push this forward?

[Noah-Kennedy]

@carllerche @Darksonn this continues to cause folks real production incidents, and is by far the most dangerous footgun lurking in Tokio today.

I think it's time we revisited whether fixing this is really a breaking change, and prepare a plan to deprecate all existing APIs that have this behavior if we cannot fix this.

AFAICT this seems to have been the behavior in Tokio 0.2.x. I do not know why we changed it, but I'd argue that this current behavior is clearly a bug.

I'm all for creating an unsafe or "discouraged" interface that gives users a blocking socket and has plenty of warnings, but I think the current behavior needs to go.

I think this is no more a breaking change than changing how we set the number of worker threads was. Both are behavioral changes which bring Tokio closer into alignment with how users think our system works, and both fix repeated causes of serious incidents which our users need to constantly guard against, and both behavioral changes are for things where there is no legitimate reason to depend on the current behavior. Everyone who hits the current behavior views this as a bug and is upset because they wasted their time, or worse, had a production incident.

If we cannot change this then we should deprecate the existing APIs (not sure what to do with TryFrom though) and have new ones which do the right thing that we can tell folks to use.