-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
net: warn on creating a tokio socket from a blocking one #7166
base: master
Are you sure you want to change the base?
Conversation
Windows does not support this, so it only works on unix for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for getting this to move forward!
Is from_std_set_nonblocking
/from_std_assume_nonblocking
still the long term plan?
Or will it instead be something where tokio sets it to nonblocking, like axum_server currently does?
Or will the panic be considered to be enough?
I think there's a bit that needs figured out still about what we do going forward. The trouble with the prior suggestion is that we actually still have the issue of I think our two long-term options here are going to be to either panic, or set the socket option silently, and there's good arguments to be made for both. There should also be something along the lines of Given that we either need to leave the footgun in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also needs review from Carl before it can be merged.
|
||
#[cfg(unix)] | ||
#[allow(unused_variables)] | ||
pub(crate) fn check_socket_for_blocking<S: AsFd>(s: &S) -> crate::io::Result<()> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this return io::Result
? It doesn't seem like the fn needs a return argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the nonblocking check fails, propating the error seems the most straightforward response.
tokio/src/net/tcp/listener.rs
Outdated
@@ -5,6 +5,7 @@ cfg_not_wasi! { | |||
use crate::net::{to_socket_addrs, ToSocketAddrs}; | |||
} | |||
|
|||
use crate::util::blocking_check::check_socket_for_blocking; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very minor and not a needed change to merge, but IMO this check makes more sense in the net
module.
07f2dd9
to
31f24d8
Compare
@carllerche @Darksonn I have updated the PR and addressed the feedback. This is ready for re-review. |
See #5595. This adds a warning when constructing a tokio socket object from an underlying std socket that is not set to nonblocking mode. This warning is likely to incrementally turn into a hard error in the future.
See #5595 and #7172.
This adds a warning when constructing a tokio socket object from an underlying std socket that is not set to nonblocking mode.
This warning is likely to incrementally turn into a hard error in the future.