Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 12 additions & 5 deletions client/network/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver, TracingUnbound
use std::{
borrow::Cow,
collections::HashSet,
fs, io,
fs,
marker::PhantomData,
pin::Pin,
str,
Expand Down Expand Up @@ -991,10 +991,17 @@ impl Metrics {
}
}

impl<B: BlockT + 'static, H: ExHashT> Future for NetworkWorker<B, H> {
type Output = Result<(), io::Error>;
impl<B: BlockT + 'static, H: ExHashT> NetworkWorker<B, H> {
/// Performs one action on the network, then returns.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not quite sure I follow. If I read the logic below correctly this does not perform a single action, but block until the entire NetworkWorker is done.

Based on the pull request description I am guessing that this comment describes the behavior that we would like to have in the future. If so, would you mind mentioning that in the comment?

///
/// The returned future is designed to be freely cancellable.
pub async fn next_action(&mut self) {
future::poll_fn(move |cx| Pin::new(&mut *self).poll(cx)).await
}

fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context) -> Poll<Self::Output> {
/// Implementation of `next_action`. Note that this is not an implementation of `Future` but

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is an implementation detail now, it could as well just implement future, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I precisely don't want NetworkWorker to implement Future, otherwise it's impossible to rewrite that as an async block.

/// a regular method.
fn poll(mut self: Pin<&mut Self>, cx: &mut std::task::Context) -> Poll<()> {
let this = &mut *self;

// Poll the import queue for actions to perform.
Expand All @@ -1019,7 +1026,7 @@ impl<B: BlockT + 'static, H: ExHashT> Future for NetworkWorker<B, H> {
// Process the next message coming from the `NetworkService`.
let msg = match this.from_worker.poll_next_unpin(cx) {
Poll::Ready(Some(msg)) => msg,
Poll::Ready(None) => return Poll::Ready(Ok(())),
Poll::Ready(None) => return Poll::Ready(()),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This return Poll::Ready(()) is used to signal not to poll the NetworkWorker again.

You must call this method in a loop.

With this comment from above in mind one would still continue polling the NetworkWorker, right? Am I missing something?

Poll::Pending => break,
};

Expand Down
7 changes: 4 additions & 3 deletions client/network/src/service/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ fn build_test_full_node(config: config::NetworkConfiguration)
None,
));

let worker = NetworkWorker::new(config::Params {
let mut worker = NetworkWorker::new(config::Params {
role: config::Role::Full,
executor: None,
network_config: config,
Expand All @@ -109,8 +109,9 @@ fn build_test_full_node(config: config::NetworkConfiguration)
let event_stream = service.event_stream("test");

async_std::task::spawn(async move {
futures::pin_mut!(worker);
let _ = worker.await;
loop {
worker.next_action().await;
}
});

(service, event_stream)
Expand Down
8 changes: 6 additions & 2 deletions client/network/test/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -789,8 +789,12 @@ pub trait TestNetFactory: Sized {
self.mut_peers(|peers| {
for peer in peers {
trace!(target: "sync", "-- Polling {}", peer.id());
if let Poll::Ready(res) = Pin::new(&mut peer.network).poll(cx) {
res.unwrap();
loop {
let net_poll_future = peer.network.next_action();
futures::pin_mut!(net_poll_future);
if let Poll::Pending = net_poll_future.poll(cx) {
break;
}
}
trace!(target: "sync", "-- Polling complete {}", peer.id());

Expand Down
6 changes: 3 additions & 3 deletions client/service/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -445,9 +445,9 @@ fn build_network_future<
});

// Main network polling.
if let Poll::Ready(Ok(())) = Pin::new(&mut network).poll(cx).map_err(|err| {
warn!(target: "service", "Error in network: {:?}", err);
}) {
let next_network_action = network.next_action();
futures::pin_mut!(next_network_action);
if let Poll::Ready(()) = next_network_action.poll(cx) {
return Poll::Ready(());
}

Expand Down