Make state conversions more explicit

Change the return type of the various `into_` function to `Result<State>` so in case of error we return `Err` instead of silently ignoring it and returning `State::Terminated`.
maidsafe · May 8, 2019 · 2453690 · 2453690
1 parent af1831f
commit 2453690
Show file tree

Hide file tree

Showing 8 changed files with 107 additions and 87 deletions.
diff --git a/src/client.rs b/src/client.rs
@@ -81,7 +81,8 @@ impl Client {
                     min_section_size,
                     timer,
                 )
-                .map_or(State::Terminated, State::BootstrappingPeer)
+                .map(State::BootstrappingPeer)
+                .unwrap_or(State::Terminated)
             },
             pub_id,
             bootstrap_config,

diff --git a/src/node.rs b/src/node.rs
@@ -141,17 +141,9 @@ impl NodeBuilder {
         StateMachine::new(
             move |action_sender, crust_service, timer, outbox2| {
                 if self.first {
-                    if let Some(state) = states::Node::first(
-                        self.cache,
-                        crust_service,
-                        full_id,
-                        min_section_size,
-                        timer,
-                    ) {
-                        State::Node(state)
-                    } else {
-                        State::Terminated
-                    }
+                    states::Node::first(self.cache, crust_service, full_id, min_section_size, timer)
+                        .map(State::Node)
+                        .unwrap_or(State::Terminated)
                 } else if !dev_config.allow_multiple_lan_nodes && crust_service.has_peers_on_lan() {
                     error!(
                         "More than one routing node found on LAN. Currently this is not supported."
@@ -168,7 +160,8 @@ impl NodeBuilder {
                         min_section_size,
                         timer,
                     )
-                    .map_or(State::Terminated, State::BootstrappingPeer)
+                    .map(State::BootstrappingPeer)
+                    .unwrap_or(State::Terminated)
                 }
             },
             pub_id,

diff --git a/src/state_machine.rs b/src/state_machine.rs
@@ -157,13 +157,31 @@ impl State {
         )
     }
 
-    fn replace_with<F>(&mut self, f: F)
+    fn replace_with<F, E>(&mut self, f: F)
     where
-        F: FnOnce(Self) -> Self,
+        F: FnOnce(Self) -> Result<Self, E>,
+        E: Debug,
     {
         let old_state = mem::replace(self, State::Terminated);
-        let new_state = f(old_state);
-        *self = new_state;
+        let old_state_log_ident = format!("{}", old_state);
+
+        match f(old_state) {
+            Ok(new_state) => *self = new_state,
+            Err(error) => error!(
+                "{} - Failed state transition: {:?}",
+                old_state_log_ident, error
+            ),
+        }
+    }
+}
+
+impl Display for State {
+    fn fmt(&self, formatter: &mut Formatter) -> fmt::Result {
+        state_dispatch!(
+            *self,
+            ref state => write!(formatter, "{}", state),
+            Terminated => write!(formatter, "Terminated")
+        )
     }
 }
 
@@ -575,6 +593,6 @@ impl StateMachine {
 
 impl Display for StateMachine {
     fn fmt(&self, formatter: &mut Formatter) -> fmt::Result {
-        self.state.fmt(formatter)
+        write!(formatter, "{:?}", self.state)
     }
 }
diff --git a/src/states/bootstrapping_peer.rs b/src/states/bootstrapping_peer.rs
@@ -74,19 +74,20 @@ impl BootstrappingPeer {
         full_id: FullId,
         min_section_size: usize,
         timer: Timer,
-    ) -> Option<Self> {
+    ) -> Result<Self, RoutingError> {
         match target_state {
             TargetState::Client { .. } => {
                 let _ = crust_service.start_bootstrap(HashSet::new(), CrustUser::Client);
             }
             TargetState::RelocatingNode | TargetState::ProvingNode { .. } => {
                 if let Err(error) = crust_service.start_listening_tcp() {
                     error!("Failed to start listening: {:?}", error);
-                    return None;
+                    return Err(error.into());
                 }
             }
         }
-        Some(Self {
+
+        Ok(Self {
             action_sender,
             cache: cache,
             crust_service,
@@ -99,19 +100,25 @@ impl BootstrappingPeer {
         })
     }
 
-    pub fn into_target_state(self, proxy_pub_id: PublicId, outbox: &mut EventBox) -> State {
+    pub fn into_target_state(
+        self,
+        proxy_pub_id: PublicId,
+        outbox: &mut EventBox,
+    ) -> Result<State, RoutingError> {
         match self.target_state {
-            TargetState::Client { msg_expiry_dur } => State::Client(Client::from_bootstrapping(
-                ClientDetails {
-                    crust_service: self.crust_service,
-                    full_id: self.full_id,
-                    min_section_size: self.min_section_size,
-                    msg_expiry_dur,
-                    proxy_pub_id,
-                    timer: self.timer,
-                },
-                outbox,
-            )),
+            TargetState::Client { msg_expiry_dur } => {
+                Ok(State::Client(Client::from_bootstrapping(
+                    ClientDetails {
+                        crust_service: self.crust_service,
+                        full_id: self.full_id,
+                        min_section_size: self.min_section_size,
+                        msg_expiry_dur,
+                        proxy_pub_id,
+                        timer: self.timer,
+                    },
+                    outbox,
+                )))
+            }
             TargetState::RelocatingNode => {
                 let details = RelocatingNodeDetails {
                     action_sender: self.action_sender,
@@ -123,12 +130,12 @@ impl BootstrappingPeer {
                     timer: self.timer,
                 };
 
-                if let Some(node) = RelocatingNode::from_bootstrapping(details) {
-                    State::RelocatingNode(node)
-                } else {
-                    outbox.send_event(Event::RestartRequired);
-                    State::Terminated
-                }
+                RelocatingNode::from_bootstrapping(details)
+                    .map(State::RelocatingNode)
+                    .map_err(|err| {
+                        outbox.send_event(Event::RestartRequired);
+                        err
+                    })
             }
             TargetState::ProvingNode {
                 old_full_id,
@@ -147,7 +154,9 @@ impl BootstrappingPeer {
                     timer: self.timer,
                 };
 
-                State::ProvingNode(ProvingNode::from_bootstrapping(details, outbox))
+                Ok(State::ProvingNode(ProvingNode::from_bootstrapping(
+                    details, outbox,
+                )))
             }
         }
     }
@@ -434,7 +443,8 @@ mod tests {
                         min_section_size,
                         timer,
                     )
-                    .map_or(State::Terminated, State::BootstrappingPeer)
+                    .map(State::BootstrappingPeer)
+                    .unwrap_or(State::Terminated)
                 },
                 pub_id,
                 Some(config),

diff --git a/src/states/establishing_node.rs b/src/states/establishing_node.rs
@@ -92,7 +92,7 @@ impl EstablishingNode {
             crust_service: details.crust_service,
             full_id: details.full_id,
             gen_pfx_info: details.gen_pfx_info,
-            msg_backlog: vec![],
+            msg_backlog: details.msg_backlog,
             notified_nodes: details.notified_nodes,
             parsec_map,
             peer_mgr: details.peer_mgr,
@@ -101,18 +101,14 @@ impl EstablishingNode {
             poke_timer_token,
         };
 
-        node.init(details.msg_backlog, outbox)?;
+        node.init(outbox)?;
         Ok(node)
     }
 
-    fn init(
-        &mut self,
-        msg_backlog: Vec<RoutingMessage>,
-        outbox: &mut EventBox,
-    ) -> Result<(), RoutingError> {
+    fn init(&mut self, outbox: &mut EventBox) -> Result<(), RoutingError> {
         debug!("{} - State changed to EstablishingNode.", self);
 
-        for msg in msg_backlog {
+        for msg in self.msg_backlog.drain(..).collect_vec() {
             let _ = self.dispatch_routing_message(msg, outbox)?;
         }
 
@@ -124,7 +120,7 @@ impl EstablishingNode {
         sec_info: SectionInfo,
         old_pfx: Prefix<XorName>,
         outbox: &mut EventBox,
-    ) -> State {
+    ) -> Result<State, RoutingError> {
         let details = NodeDetails {
             ack_mgr: self.ack_mgr,
             cache: self.cache,
@@ -140,10 +136,7 @@ impl EstablishingNode {
             timer: self.timer,
         };
 
-        match Node::from_establishing_node(details, sec_info, old_pfx, outbox) {
-            Ok(node) => State::Node(node),
-            Err(_) => State::Terminated,
-        }
+        Node::from_establishing_node(details, sec_info, old_pfx, outbox).map(State::Node)
     }
 
     fn dispatch_routing_message(

diff --git a/src/states/node.rs b/src/states/node.rs
@@ -142,12 +142,12 @@ impl Node {
         full_id: FullId,
         min_section_size: usize,
         timer: Timer,
-    ) -> Option<Self> {
+    ) -> Result<Self, RoutingError> {
         let dev_config = config_handler::get_config().dev.unwrap_or_default();
 
         let public_id = *full_id.public_id();
         let gen_pfx_info = GenesisPfxInfo {
-            first_info: create_first_section_info(public_id).ok()?,
+            first_info: create_first_section_info(public_id)?,
             latest_info: SectionInfo::default(),
         };
         let parsec_map = ParsecMap::new(full_id.clone(), &gen_pfx_info);
@@ -171,13 +171,16 @@ impl Node {
 
         let mut node = Self::new(details, true);
 
-        if let Err(error) = node.crust_service.start_listening_tcp() {
-            error!("{} - Failed to start listening: {:?}", node, error);
-            None
-        } else {
-            debug!("{} - State changed to Node.", node);
-            info!("{} - Started a new network as a seed node.", node);
-            Some(node)
+        match node.crust_service.start_listening_tcp() {
+            Ok(()) => {
+                debug!("{} - State changed to Node.", node);
+                info!("{} - Started a new network as a seed node.", node);
+                Ok(node)
+            }
+            Err(error) => {
+                error!("{} - Failed to start listening: {:?}", node, error);
+                Err(error.into())
+            }
         }
     }
 

diff --git a/src/states/proving_node.rs b/src/states/proving_node.rs
@@ -150,7 +150,7 @@ impl ProvingNode {
         self,
         gen_pfx_info: GenesisPfxInfo,
         outbox: &mut EventBox,
-    ) -> State {
+    ) -> Result<State, RoutingError> {
         let details = EstablishingNodeDetails {
             ack_mgr: self.ack_mgr,
             cache: self.cache,
@@ -165,10 +165,7 @@ impl ProvingNode {
             timer: self.timer,
         };
 
-        match EstablishingNode::from_proving_node(details, outbox) {
-            Ok(node) => State::EstablishingNode(node),
-            Err(_) => State::Terminated,
-        }
+        EstablishingNode::from_proving_node(details, outbox).map(State::EstablishingNode)
     }
 
     fn dispatch_routing_message(