From 8637003572e630cbd6f201d39e3ab797e862e2c9 Mon Sep 17 00:00:00 2001 From: Robb Walters Date: Tue, 16 Jun 2020 19:31:30 -0700 Subject: [PATCH 1/6] why lock? --- consensus/scp/tests/mock_network/mod.rs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/consensus/scp/tests/mock_network/mod.rs b/consensus/scp/tests/mock_network/mod.rs index d3d441eee4..ce9c696070 100644 --- a/consensus/scp/tests/mock_network/mod.rs +++ b/consensus/scp/tests/mock_network/mod.rs @@ -387,8 +387,6 @@ impl SimulatedNode { let outgoing_msg: Option> = { thread_local_node - .lock() - .expect("thread_local_node lock failed when nominating value") .nominate( current_slot as SlotIndex, BTreeSet::from_iter(values_to_nominate) @@ -407,8 +405,6 @@ impl SimulatedNode { for msg in incoming_msgs.iter() { let outgoing_msg: Option> = { thread_local_node - .lock() - .expect("thread_local_node lock failed when handling msg") .handle(msg) .expect("node.handle_msg() failed") }; @@ -422,8 +418,6 @@ impl SimulatedNode { // Process timeouts (for all slots) let timeout_msgs: Vec> = { thread_local_node - .lock() - .expect("thread_local_node lock failed when processing timeouts") .process_timeouts() .into_iter() .collect() @@ -436,8 +430,6 @@ impl SimulatedNode { // Check if the current slot is done let new_block:Vec = { thread_local_node - .lock() - .expect("thread_local_node lock failed when collecting externalized values") .get_externalized_values(current_slot as SlotIndex) }; From f31d65fe2726553eb31f378b49801e9b8f2a8456 Mon Sep 17 00:00:00 2001 From: Robb Walters Date: Tue, 16 Jun 2020 19:43:07 -0700 Subject: [PATCH 2/6] Revert "why lock?" This reverts commit 8637003572e630cbd6f201d39e3ab797e862e2c9. --- consensus/scp/tests/mock_network/mod.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/consensus/scp/tests/mock_network/mod.rs b/consensus/scp/tests/mock_network/mod.rs index ce9c696070..d3d441eee4 100644 --- a/consensus/scp/tests/mock_network/mod.rs +++ b/consensus/scp/tests/mock_network/mod.rs @@ -387,6 +387,8 @@ impl SimulatedNode { let outgoing_msg: Option> = { thread_local_node + .lock() + .expect("thread_local_node lock failed when nominating value") .nominate( current_slot as SlotIndex, BTreeSet::from_iter(values_to_nominate) @@ -405,6 +407,8 @@ impl SimulatedNode { for msg in incoming_msgs.iter() { let outgoing_msg: Option> = { thread_local_node + .lock() + .expect("thread_local_node lock failed when handling msg") .handle(msg) .expect("node.handle_msg() failed") }; @@ -418,6 +422,8 @@ impl SimulatedNode { // Process timeouts (for all slots) let timeout_msgs: Vec> = { thread_local_node + .lock() + .expect("thread_local_node lock failed when processing timeouts") .process_timeouts() .into_iter() .collect() @@ -430,6 +436,8 @@ impl SimulatedNode { // Check if the current slot is done let new_block:Vec = { thread_local_node + .lock() + .expect("thread_local_node lock failed when collecting externalized values") .get_externalized_values(current_slot as SlotIndex) }; From a5f9bc4100df6c780a63b3bc8e07a9fee84ffd4a Mon Sep 17 00:00:00 2001 From: Robb Walters Date: Tue, 16 Jun 2020 19:45:05 -0700 Subject: [PATCH 3/6] add locks on node --- consensus/scp/tests/mock_network/mod.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/consensus/scp/tests/mock_network/mod.rs b/consensus/scp/tests/mock_network/mod.rs index d3d441eee4..cfdc09534d 100644 --- a/consensus/scp/tests/mock_network/mod.rs +++ b/consensus/scp/tests/mock_network/mod.rs @@ -191,6 +191,8 @@ impl SimulatedNetwork { nodes_map .get_mut(&test_utils::test_node_id(node_num as u32)) .expect("failed to get node from nodes_map") + .lock() + .expect("lock failed on node sending stop") .send_stop(); } drop(nodes_map); @@ -213,6 +215,8 @@ impl SimulatedNetwork { .expect("lock failed on nodes_map getting node") .get(node_id) .expect("could not find node_id in nodes_map") + .lock() + .expect("lock failed on node sending value") .send_value(value); } @@ -245,7 +249,7 @@ impl SimulatedNetwork { .lock() .expect("lock failed on nodes_map in broadcast"); - log::trace!(logger, "(broadcast) {}", msg.to_display(),); + log::trace!(logger, "(broadcast) {}", msg.to_display()); let amsg = Arc::new(msg); From 88555552c042305505f39a115b152fb8521c0914 Mon Sep 17 00:00:00 2001 From: Robb Walters Date: Tue, 16 Jun 2020 19:51:38 -0700 Subject: [PATCH 4/6] local node isn't used outside thread --- consensus/scp/tests/mock_network/mod.rs | 42 +++++++++---------------- 1 file changed, 14 insertions(+), 28 deletions(-) diff --git a/consensus/scp/tests/mock_network/mod.rs b/consensus/scp/tests/mock_network/mod.rs index cfdc09534d..7bce2f5053 100644 --- a/consensus/scp/tests/mock_network/mod.rs +++ b/consensus/scp/tests/mock_network/mod.rs @@ -288,7 +288,6 @@ impl SimulatedNodeSharedData { // A simulated validator node struct SimulatedNode { - local_node: Arc>>, sender: crossbeam_channel::Sender, shared_data: Arc>, } @@ -303,27 +302,22 @@ impl SimulatedNode { logger: Logger, ) -> (Self, Option>) { let (sender, receiver) = crossbeam_channel::unbounded(); - let local_node = Arc::new(Mutex::new(Node::new( + + let simulated_node = Self { + sender, + shared_data: Arc::new(Mutex::new(SimulatedNodeSharedData { ledger: Vec::new() })), + }; + + let thread_local_node = Node::new( node_id.clone(), quorum_set, test_options.validity_fn.clone(), test_options.combine_fn.clone(), logger.clone(), - ))); - - local_node - .lock() - .expect("lock failed on local node setting scp_timebase_millis") - .scp_timebase = test_options.scp_timebase; - - let node = Self { - local_node, - sender, - shared_data: Arc::new(Mutex::new(SimulatedNodeSharedData { ledger: Vec::new() })), - }; + ); + thread_local_node.scp_timebase = test_options.scp_timebase; - let thread_shared_data = Arc::clone(&node.shared_data); - let thread_local_node = Arc::clone(&node.local_node); + let thread_shared_data = Arc::clone(&simulated_node.shared_data); // See byzantine_ledger.rs#L626 let max_pending_values_to_nominate: usize = test_options.max_pending_values_to_nominate; @@ -391,13 +385,11 @@ impl SimulatedNode { let outgoing_msg: Option> = { thread_local_node - .lock() - .expect("thread_local_node lock failed when nominating value") .nominate( current_slot as SlotIndex, BTreeSet::from_iter(values_to_nominate) ) - .expect("node.nominate() failed") + .expect("nominate() failed") }; if let Some(outgoing_msg) = outgoing_msg { @@ -411,10 +403,8 @@ impl SimulatedNode { for msg in incoming_msgs.iter() { let outgoing_msg: Option> = { thread_local_node - .lock() - .expect("thread_local_node lock failed when handling msg") .handle(msg) - .expect("node.handle_msg() failed") + .expect("handle_msg() failed") }; if let Some(outgoing_msg) = outgoing_msg { @@ -426,8 +416,6 @@ impl SimulatedNode { // Process timeouts (for all slots) let timeout_msgs: Vec> = { thread_local_node - .lock() - .expect("thread_local_node lock failed when processing timeouts") .process_timeouts() .into_iter() .collect() @@ -440,9 +428,7 @@ impl SimulatedNode { // Check if the current slot is done let new_block:Vec = { thread_local_node - .lock() - .expect("thread_local_node lock failed when collecting externalized values") - .get_externalized_values(current_slot as SlotIndex) + .get_externalized_values(current_slot as SlotIndex) }; if !new_block.is_empty() { @@ -488,7 +474,7 @@ impl SimulatedNode { .expect("failed spawning SimulatedNode thread"), ); - (node, thread_handle) + (simulated_node, thread_handle) } /// Push value to this node's consensus task. From 14845295d4669370923fe32b2d34f3534c82fdc9 Mon Sep 17 00:00:00 2001 From: Robb Walters Date: Tue, 16 Jun 2020 19:54:46 -0700 Subject: [PATCH 5/6] cleanup simulatednode locks --- consensus/scp/tests/mock_network/mod.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/consensus/scp/tests/mock_network/mod.rs b/consensus/scp/tests/mock_network/mod.rs index 7bce2f5053..61a9dd0f77 100644 --- a/consensus/scp/tests/mock_network/mod.rs +++ b/consensus/scp/tests/mock_network/mod.rs @@ -190,9 +190,7 @@ impl SimulatedNetwork { for node_num in 0..num_nodes { nodes_map .get_mut(&test_utils::test_node_id(node_num as u32)) - .expect("failed to get node from nodes_map") - .lock() - .expect("lock failed on node sending stop") + .expect("could not find node_id in nodes_map") .send_stop(); } drop(nodes_map); @@ -212,18 +210,16 @@ impl SimulatedNetwork { fn push_value(&self, node_id: &NodeID, value: &str) { self.nodes_map .lock() - .expect("lock failed on nodes_map getting node") + .expect("lock failed on nodes_map pushing value") .get(node_id) .expect("could not find node_id in nodes_map") - .lock() - .expect("lock failed on node sending value") .send_value(value); } fn get_ledger(&self, node_id: &NodeID) -> Vec> { self.nodes_shared_data .get(node_id) - .expect("could not find node_id in nodes_map") + .expect("could not find node_id in nodes_shared_data") .lock() .expect("lock failed on shared_data getting ledger") .ledger @@ -233,7 +229,7 @@ impl SimulatedNetwork { fn get_ledger_size(&self, node_id: &NodeID) -> usize { self.nodes_shared_data .get(node_id) - .expect("could not find node_id in nodes_map") + .expect("could not find node_id in nodes_shared_data") .lock() .expect("lock failed on shared_data getting ledger size") .ledger_size() From f734845e84605c89e74e4747da117a50369ae8ea Mon Sep 17 00:00:00 2001 From: Robb Walters Date: Tue, 16 Jun 2020 19:55:32 -0700 Subject: [PATCH 6/6] add mut --- consensus/scp/tests/mock_network/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/consensus/scp/tests/mock_network/mod.rs b/consensus/scp/tests/mock_network/mod.rs index 61a9dd0f77..8011d3c121 100644 --- a/consensus/scp/tests/mock_network/mod.rs +++ b/consensus/scp/tests/mock_network/mod.rs @@ -304,7 +304,7 @@ impl SimulatedNode { shared_data: Arc::new(Mutex::new(SimulatedNodeSharedData { ledger: Vec::new() })), }; - let thread_local_node = Node::new( + let mut thread_local_node = Node::new( node_id.clone(), quorum_set, test_options.validity_fn.clone(),