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

TcpListener::from_std is super easy to misuse #5595

Open
Ten0 opened this issue Apr 3, 2023 · 6 comments · May be fixed by #5597
Open

TcpListener::from_std is super easy to misuse #5595

Ten0 opened this issue Apr 3, 2023 · 6 comments · May be fixed by #5597
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-net Module: tokio/net

Comments

@Ten0
Copy link

Ten0 commented Apr 3, 2023

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.
@Ten0 Ten0 added A-tokio Area: The main tokio crate C-feature-request Category: A feature request. labels Apr 3, 2023
@Darksonn Darksonn added the M-net Module: tokio/net label Apr 3, 2023
@Darksonn
Copy link
Contributor

Darksonn commented Apr 3, 2023

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
Copy link
Author

Ten0 commented Apr 3, 2023

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
Copy link
Author

Ten0 commented Apr 3, 2023

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
Copy link
Contributor

Darksonn commented Apr 3, 2023

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

@Ten0
Copy link
Author

Ten0 commented Apr 3, 2023

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
Copy link
Contributor

Darksonn commented Apr 3, 2023

Yes, go ahead and open a PR.

@Ten0 Ten0 linked a pull request Apr 3, 2023 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate C-feature-request Category: A feature request. M-net Module: tokio/net
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Update naming around TcpListener::from_std so that it's harder to misuse Ten0/tokio
2 participants
@Darksonn @Ten0