-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Add Send/Sync traits on LookupHost struct #27966
Conversation
Hi! Did you find a resource that gives us confidence that this is in fact thread safe? Just wondering if you've found anything interesting. |
Not yet, it was just in order to gather people. I totally missed the last discussion you had with others so this time I hope I might be included. :) |
http://tools.ietf.org/html/rfc3493#section-6.1 says the functions are thread safe. There shouldn't be much of an issue, we just need to treat the returned pointer to linked list as an owned pointer. LookupHost is trivially Sync because we can't access anything of its internals through a &self. |
Well, I guess it will be quick to check then. |
@alexcrichton: Hold on ! Shouldn't I add tests ? |
Sure, feel free to add an amendment which adds those tests. |
Wow this took quite a while to happen. |
@nagisa: It took me a while to find what I was looking for. ^^' |
@bors: r- (gonna add a test) |
ec13e1b
to
528d99c
Compare
($ctor:expr, $($iter:ident),+) => ({ | ||
$( | ||
let mut x = $ctor; | ||
is_sync(x.$iter()); |
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 seems wrong. You're testing whether the return value of <LookupHost as Iterator>::next
is Send/Sync. Shouldn't you be testing LookupHost
directly? That would be IntoIter<SocketAddr>
now, I think.
Part of #22709.
cc @Veedrac
r? @bluss
I don't have added tests yet, I'll see how to do it tomorrow.