Skip to content

Commit

Permalink
fix: race in NetworkRecepient
Browse files Browse the repository at this point in the history
closes #6826

We have circular depedencies between PeerManager and Client, which we
break via `dyn PeerManagerAdapter`. As this is a real cycle, we close it
at runtime by calling `set_recipient`. But today nothing prevents client
from sending message to network before that, and indeed that happens if
we are unlucky with timing.

This commit fixes the issue by making `PeerManagerAdapter` APIs block
until the loop is closed.
  • Loading branch information
matklad committed May 20, 2022
1 parent 010b32f commit 09ba69f
Show file tree
Hide file tree
Showing 3 changed files with 11 additions and 24 deletions.
4 changes: 1 addition & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ version = "0.13.0"
exclude = [ "neard" ]

[patch.crates-io]
once_cell = {path="/home/matklad/p/once_cell"}

# Note that "bench" profile inherits from "release" profile and
# "test" profile inherits from "dev" profile.
Expand Down
30 changes: 9 additions & 21 deletions chain/network/src/test_utils.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use near_primitives::hash::hash;
use near_primitives::network::PeerId;
use near_primitives::types::EpochId;
use near_primitives::utils::index_to_bytes;
use once_cell::sync::Lazy;
use once_cell::sync::{Lazy, OnceCell};
use rand::{thread_rng, RngCore};
use std::collections::{HashMap, HashSet, VecDeque};
use std::net::TcpListener;
Expand Down Expand Up @@ -387,39 +387,27 @@ pub mod test_features {
}
}

#[derive(Default)]
pub struct NetworkRecipient {
peer_manager_recipient: OnceCell<Recipient<PeerManagerMessageRequest>>,
}

impl NetworkRecipient {
pub fn set_recipient(&self, peer_manager_recipient: Recipient<PeerManagerMessageRequest>) {
*self.peer_manager_recipient.write().unwrap() = Some(peer_manager_recipient);
self.peer_manager_recipient.set(peer_manager_recipient);
}
}

#[derive(Default)]
pub struct NetworkRecipient {
peer_manager_recipient: RwLock<Option<Recipient<PeerManagerMessageRequest>>>,
}

impl PeerManagerAdapter for NetworkRecipient {
fn send(
&self,
msg: PeerManagerMessageRequest,
) -> BoxFuture<'static, Result<PeerManagerMessageResponse, MailboxError>> {
self.peer_manager_recipient
.read()
.unwrap()
.as_ref()
.expect("Recipient must be set")
.send(msg)
.boxed()
self.peer_manager_recipient.wait().send(msg).boxed()
}

fn do_send(&self, msg: PeerManagerMessageRequest) {
let _ = self
.peer_manager_recipient
.read()
.unwrap()
.as_ref()
.expect("Recipient must be set")
.do_send(msg);
let _ = self.peer_manager_recipient.wait().do_send(msg);
}
}

Expand Down

0 comments on commit 09ba69f

Please sign in to comment.