-
Notifications
You must be signed in to change notification settings - Fork 34
DHT refactoring #161
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
DHT refactoring #161
Conversation
43bf43f
to
95845d1
Compare
examples/dht_server.rs
Outdated
let server = server.join(lossless_handler).map(|_| ()); | ||
let server = server.join(lossy_handler).map(|_| ()); | ||
let server: IoFuture<()> = Box::new(network); // TODO: remove these boxes on rustc 1.26 | ||
let server: IoFuture<()> = Box::new(server.join(run_server(&server_obj)).map(|_| ())); |
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.
use select instead of join
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.
Could you explain why you think that select
is should be used here? With select
you must assume that all these futures are infinite whereas join
explicitly shows that application will be terminated when all these futures are completed.
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.
Because if any of the futures resolves, we should return immediately. In your case we will wait until all futures are terminated.
Right now all these futures are infinite so it is better to use .select
. If the server stops with a resolved future we will notice that some of these futures resolved and stopped the server.
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.
But in case if one future resolves with error join
won't wait other futures.
And another argument for join
- select
modifies error type to either of errors, so I can't just change join
to select
, I'll have to add map_err
for every line here.
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.
A simple rule:
- use
select
if you don't have to wait for the other futures to resolve. - use
join
if you need to wait for them.
Do we need to wait for other futures?
src/toxcore/dht/lan_discovery.rs
Outdated
use futures::{Future, Stream}; | ||
|
||
macro_rules! unpack { | ||
($variable:expr, $variant:path) => ( |
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.
How about moving this macro definition to binary_io.rs.
Because this macro is used commonly in many files.
4629143
to
4de04da
Compare
4de04da
to
58dd55c
Compare
This PR:
friend_connection.c
dht_main_loop
I decided to leave LAN discovery wake ups separated from
dht_main_loop
since: