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.

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

Use quinn 0.5.0 and async/await code #82

Merged
merged 6 commits into from Jan 14, 2020
Merged

Conversation

@nbaksalyar
Copy link
Member

nbaksalyar commented Dec 22, 2019

Also upgrade tokio to 0.5.2 and futures to 0.3.
Supersedes and includes changes from #80 and #79.

@nbaksalyar nbaksalyar requested a review from maidsafe/backend_codeowners as a code owner Dec 22, 2019
Copy link
Member

dirvine left a comment

Looks great, just some concern with handling result, esp where we spawn async workers.


current_thread::spawn(leaf);
let _ = tokio::spawn(leaf);

This comment has been minimized.

Copy link
@dirvine

dirvine Dec 24, 2019

Member

Is this _ pattern required for tokio?

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jan 8, 2020

Author Member

Yes, tokio::spawn returns a JoinHandle now which is similar to a handle returned by thread::spawn. We don't have a use for it, hence let _ = ...

let err = match res {
Err(e) => {
debug!(
"Error in Incoming-bi-stream while reading from peer {}: {:?} - {}.\nNote: It

This comment has been minimized.

Copy link
@dirvine

dirvine Dec 24, 2019

Member

Although bi-directional are allowed for clients? Perhaps reword to make that clear?

This comment has been minimized.

Copy link
@dirvine

dirvine Dec 24, 2019

Member

i.e. p2p nodes cannot be connected bi-directionally. Client connections are bi-directional, but p2p nodes cannot be.

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jan 8, 2020

Author Member

Although bi-directional are allowed for clients?

Not quite, it's a bit confusing thing, but we don't use bi-directional streams at all. For clients it's more like 2 uni-directional streams instead of 1 bi-directional.

))
}
}

impl Default for SerialisableCertificate {
fn default() -> Self {
let cert = rcgen::generate_simple_self_signed(vec!["MaidSAFE.net".to_string()]);
let cert = unwrap!(rcgen::generate_simple_self_signed(vec![

This comment has been minimized.

Copy link
@dirvine

dirvine Dec 24, 2019

Member

We need to handle this error.

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jan 8, 2020

Author Member

Might need a slight code rewrite here because this one is in the Default impl which doesn't return a Result, so I'd prefer to tackle it in a separate PR along with quick_error removal if you don't mind?


Self {
cert_der: cert.serialize_der(),
cert_der: unwrap!(cert.serialize_der()),

This comment has been minimized.

Copy link
@dirvine

dirvine Dec 24, 2019

Member

Same as above.

@@ -123,12 +117,10 @@ fn spawn_incomplete_conn_killer(peer_addr: SocketAddr) {
let _ = conn.remove();
}
});

Ok(())
});

// TODO find a way to cancel this timer if we know the connection is done. Otherwise it

This comment has been minimized.

Copy link
@dirvine

dirvine Dec 24, 2019

Member

Can we make sure this is a GH issue and give it some priority

@@ -100,5 +100,21 @@ quick_error! {
InvalidWireMsgFlag {
display("Could not deserialise a wire message: unexpected message type flag")
}
/// Write error

This comment has been minimized.

Copy link
@dirvine

dirvine Dec 24, 2019

Member

I will do a follow-up PR to remove quick_error and we will use err-derive as quinn does. That should give us composable errors based on std::Error

});
};

let mut runtime = unwrap!(tokio::runtime::Runtime::new());

This comment has been minimized.

Copy link
@dirvine

dirvine Dec 24, 2019

Member

Here if we cannot start runtime we may need to panic, but with a clear message. We can add description to unwrap here AFAIK `unwrap!(tokio::runtime::Runtime::new(), "Cannot start quick-p2p runtime, aborting")


let mut peer_cfg_builder = {
let mut client_cfg = quinn::ClientConfig::default();
client_cfg.transport = Arc::new(new_transport_cfg(None, None));

quinn::ClientConfigBuilder::new(client_cfg)
};
let _ = peer_cfg_builder.add_certificate_authority(peer_cert)?;
let _ = peer_cfg_builder

This comment has been minimized.

Copy link
@dirvine

dirvine Dec 24, 2019

Member

We should handle this result.

This comment has been minimized.

Copy link
@nbaksalyar

nbaksalyar Jan 8, 2020

Author Member

This one should be handled: it goes like let _ = peer_cfg_builder.map_err(..)?, so the discarded result here is only Ok(ConfigBuilder) which we have no use for.

let err = match res {
Err(e) => {
debug!(
"Error in Incoming-bi-stream while reading from peer {}: {:?} - {}.\nNote: It

This comment has been minimized.

Copy link
@octol

octol Jan 1, 2020

Contributor

Seems like there are some tab whitespace here?

new_peer_conn_res = connecting.fuse() => {
handle_new_connection_res(peer_addr, new_peer_conn_res);
},
}

This comment has been minimized.

Copy link
@octol

octol Jan 1, 2020

Contributor

Nitpick: indentation?

@nbaksalyar nbaksalyar force-pushed the nbaksalyar:new-quinn branch from c5e5aa7 to 66a8a5b Jan 8, 2020
@nbaksalyar nbaksalyar force-pushed the nbaksalyar:new-quinn branch from 66a8a5b to 08d5984 Jan 8, 2020
@octol
octol approved these changes Jan 8, 2020
@nbaksalyar nbaksalyar merged commit c8fc50a into maidsafe:master Jan 14, 2020
5 checks passed
5 checks passed
Rustfmt-Clippy
Details
Test (ubuntu-latest)
Details
Test (windows-latest)
Details
Test (macOS-latest)
Details
Test Publish
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.