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 dependencies 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 23, 2022
1 parent 010b32f commit a6d3594
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 28 deletions.
12 changes: 6 additions & 6 deletions Cargo.lock

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

2 changes: 1 addition & 1 deletion chain/network/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ itertools = "0.10.3"
lru = "0.7.2"
thiserror = "1"
near-rust-allocator-proxy = { version = "0.4", optional = true }
once_cell = "1.5.2"
once_cell = "1.12.0"
rand = "0.6"
rand_pcg = "0.1"
serde = { version = "1", features = ["alloc", "derive", "rc"], optional = true }
Expand Down
32 changes: 11 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,29 @@ 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)
.expect("can't `set_recipient` twice");
}
}

#[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 a6d3594

Please sign in to comment.