From 5b43e28eba3cb81d3122fe7d62737a2f51e174d6 Mon Sep 17 00:00:00 2001 From: Chris O'Neil Date: Sat, 27 Aug 2022 16:35:33 +0100 Subject: [PATCH] feat: use err result to send errors back to client Previously the mechanism for handling errors in `handle_valid_service_msg` was to return an `Ok` result with an empty list of commands, or if you wanted to send an error response back to the client, to include a command in that `Ok` result. A new `CmdProcessingClientRespondError` variant is added to `Error` to enable returning an `Err` result when we want to return an error back to the client. When the command processing code encounters this error variant, it will process the commands included in the error, which will be the information to send back to the client. The intention is also to return `Err` results from `handle_valid_service_msg` when any errors are encountered, not just for the limited few we want to send back to the client. --- sn_node/src/comm/mod.rs | 2 +- sn_node/src/node/error.rs | 13 +++++++++++-- sn_node/src/node/flow_ctrl/cmd_ctrl.rs | 14 +++++++++++++- sn_node/src/node/flow_ctrl/cmds.rs | 4 ++-- sn_node/src/node/messaging/mod.rs | 4 ++-- sn_node/src/node/proposal.rs | 4 ++-- 6 files changed, 31 insertions(+), 10 deletions(-) diff --git a/sn_node/src/comm/mod.rs b/sn_node/src/comm/mod.rs index af1680031b..18e375898e 100644 --- a/sn_node/src/comm/mod.rs +++ b/sn_node/src/comm/mod.rs @@ -53,7 +53,7 @@ pub(crate) struct Comm { /// Commands for interacting with Comm. #[allow(unused)] #[derive(Debug, Clone)] -pub(crate) enum Cmd { +pub enum Cmd { #[cfg(feature = "back-pressure")] /// Set message rate for peer to the desired msgs per second Regulate { peer: Peer, msgs_per_s: f64 }, diff --git a/sn_node/src/node/error.rs b/sn_node/src/node/error.rs index 82649d267c..cdee5fbe28 100644 --- a/sn_node/src/node/error.rs +++ b/sn_node/src/node/error.rs @@ -8,12 +8,12 @@ use super::Prefix; -use crate::node::handover::Error as HandoverError; +use crate::node::{flow_ctrl::cmds::Cmd, handover::Error as HandoverError}; use sn_dbc::Error as DbcError; use sn_interface::{ messaging::data::Error as ErrorMsg, - types::{convert_dt_error_to_error_msg, DataAddress, Peer, PublicKey}, + types::{convert_dt_error_to_error_msg, Peer, PublicKey, ReplicatedDataAddress as DataAddress}, }; use secured_linked_list::error::Error as SecuredLinkedListError; @@ -203,6 +203,15 @@ pub enum Error { GenesisDbcError(String), #[error("DbcError: {0}")] DbcError(#[from] DbcError), + /// An error occurred while processing a command and we want to send a response back to the + /// client. + /// + /// It's not really a type of error as such, but rather a marker for the command processing + /// code that we want to send an acknowledgement back to the client. It enables command + /// processing (or message handling) to return errors for more robust unit testing. Previously + /// if there was an error we would return back an `Ok` result with an empty list of commands. + #[error("Error processing command. Response will be sent back to client.")] + CmdProcessingClientRespondError(Vec), } impl From for Error { diff --git a/sn_node/src/node/flow_ctrl/cmd_ctrl.rs b/sn_node/src/node/flow_ctrl/cmd_ctrl.rs index a0e67b7038..9cb3d060fb 100644 --- a/sn_node/src/node/flow_ctrl/cmd_ctrl.rs +++ b/sn_node/src/node/flow_ctrl/cmd_ctrl.rs @@ -193,13 +193,25 @@ impl CmdCtrl { .await; } Err(error) => { + if let Error::CmdProcessingClientRespondError(ref cmds) = error { + for cmd in cmds.clone() { + match cmd_process_api.send((cmd, Some(id))).await { + Ok(_) => { + //no issues + } + Err(error) => { + error!("Could not enqueue child Cmd: {:?}", error); + } + } + } + } node_event_sender .send(Event::CmdProcessing(CmdProcessEvent::Failed { id, priority, time: SystemTime::now(), cmd_string, - error: format!("{:?}", error), + error: format!("{:?}", &error.to_string()), })) .await; } diff --git a/sn_node/src/node/flow_ctrl/cmds.rs b/sn_node/src/node/flow_ctrl/cmds.rs index 600d8ce6e6..9b4c238cf4 100644 --- a/sn_node/src/node/flow_ctrl/cmds.rs +++ b/sn_node/src/node/flow_ctrl/cmds.rs @@ -102,7 +102,7 @@ impl CmdJob { } } -/// Internal cmds for a node. +/// Commands for a node. /// /// Cmds are used to connect different modules, allowing /// for a better modularization of the code base. @@ -111,7 +111,7 @@ impl CmdJob { /// In other words, it enables enhanced flow control. #[allow(clippy::large_enum_variant)] #[derive(Debug, Clone)] -pub(crate) enum Cmd { +pub enum Cmd { /// Cleanup node's PeerLinks, removing any unsused, unconnected peers CleanupPeerLinks, /// Validate `wire_msg` from `sender`. diff --git a/sn_node/src/node/messaging/mod.rs b/sn_node/src/node/messaging/mod.rs index e949436b5a..d28763f13b 100644 --- a/sn_node/src/node/messaging/mod.rs +++ b/sn_node/src/node/messaging/mod.rs @@ -35,14 +35,14 @@ use std::collections::BTreeSet; #[derive(Debug, Clone)] #[allow(clippy::large_enum_variant)] -pub(crate) enum OutgoingMsg { +pub enum OutgoingMsg { System(SystemMsg), Service(ServiceMsg), DstAggregated((BlsShareAuth, Bytes)), } #[derive(Debug, Clone)] -pub(crate) enum Peers { +pub enum Peers { Single(Peer), Multiple(BTreeSet), } diff --git a/sn_node/src/node/proposal.rs b/sn_node/src/node/proposal.rs index 40544b1da5..57636c1589 100644 --- a/sn_node/src/node/proposal.rs +++ b/sn_node/src/node/proposal.rs @@ -14,8 +14,8 @@ use sn_interface::{ }; #[allow(clippy::large_enum_variant)] -#[derive(Clone, Debug, PartialEq)] -pub(crate) enum Proposal { +#[derive(Clone, Debug, Eq, PartialEq)] +pub enum Proposal { VoteNodeOffline(NodeState), SectionInfo(SectionAuthorityProvider), NewElders(SectionAuth),