-
Notifications
You must be signed in to change notification settings - Fork 13.2k
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
Implemented shutdown for UdpSocket (fixes #23194). #23214
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see CONTRIBUTING.md for more information. |
In addition to tests, could you also refactor the implementations of |
It seems as though I cannot write any test for this that doesn't just yield |
Hm I just wrote a small test locally that passes, it looks like use std::net::UdpSocket;
use std::thread;
use std::os::unix::prelude::*;
fn main() {
let b = UdpSocket::bind("0.0.0.0:3944").unwrap();
let _t = thread::scoped(|| {
let mut b = &b;
println!("a");
assert!(b.recv_from(&mut []).is_err());
println!("here");
});
let fd = b.as_raw_fd();
extern {
fn shutdown(socket: i32, how: i32) -> i32;
fn sleep(seconds: u32) -> u32;
}
unsafe {
sleep(1);
println!("shutting down");
println!("{}", shutdown(fd as i32, 2));
println!("shut down");
}
} |
I have what appears to me to be an almost identical test that's still failing with the same issue. I must be missing something here. Hopefully, you can pick it out. I tried adding sleep just as you did, but it didn't help. #[test]
fn udp_shutdown_smoke() {
each_ip(&mut |addr, _| {
let sock = t!(UdpSocket::bind(&addr));
let _t = thread::scoped(|| {
let mut sock = &sock;
assert!(sock.recv_from(&mut []).is_err());
});
t!(sock.shutdown(Shutdown::Read));
})
} |
Hm, you may need to fiddle with the synchronization there to guarantee that the |
I had tried adding the sleep, too. It didn't make a difference. |
Hm interestingly enough it appears I wasn't even reading the output of my program! It also appears to return an error (of the same kind). Oddly enough it looks like it also unblocks any pending read (but not future reads). In light of this we may not want to expose this at this time. One can indeed |
Seems fair to me. I was wary of this to begin with because most information online seemed to indicate that shutting down a UDP socket wasn't particularly useful. |
Ok, thanks anyway for looking into this @aaxte! |
See title. This should probably have unit tests. So, I'll add those soon (a bit busy right now, but wanted to get this out) and then squash.