updates, errors: Completely revamp error handling

There were multiple bugs here.  We weren't coherent about what to send
to whom, and whether to update the piece generation, if there was an
error (including a conflict or a partially processed API op).

Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>

diff --git a/daemon/api.rs b/daemon/api.rs
index c0ec99a1ce334d999a6ae5b260c46d0433aaa4e4..97c3779ba96911e2bdc14889818094b5024bfc05 100644 (file)
--- a/daemon/api.rs
+++ b/daemon/api.rs
@@ -144,14 +144,14 @@ fn api_piece_op<O: ApiPieceOp>(form : Json<ApiPiece<O>>)
     Err(ReportViaUpdate(poe)) => {
       PrepareUpdatesBuffer::piece_report_error(
         &mut ig, poe,
-        piece, vec![], client, &lens
+        piece, vec![], POEPP::Unprocessed, client, form.cseq, &lens
       )?;
       debug!("api_piece_op Err(RVU): {:?}", &form);
     },
     Err(PartiallyProcessed(poe, logents)) => {
       PrepareUpdatesBuffer::piece_report_error(
         &mut ig, poe,
-        piece, logents, client, &lens
+        piece, logents, POEPP::Partially, client, form.cseq, &lens
       )?;
       debug!("api_piece_op Err(PP): {:?}", &form);
     },

diff --git a/src/error.rs b/src/error.rs
index 04e4978fea43593d5617491a5a41c9641f5af17e..8fbded1e42ed92f031046692ce4898b954ce130c 100644 (file)
--- a/src/error.rs
+++ b/src/error.rs
@@ -96,17 +96,24 @@ impl From<PlayerNotFound> for ApiPieceOpError {
 }
 
 #[derive(Error,Debug,Serialize,Clone)]
-pub enum ErrorSignaledViaUpdate {
+pub enum ErrorSignaledViaUpdate<POEPU: Debug> {
   InternalError,
-  PlayerRemoved,
-  TokenRevoked,
+  PlayerRemoved, // appears only in streams for applicable player
+  TokenRevoked, // appears only in streams for applicable player
   PieceOpError {
-    piece: VisiblePieceId,
     error: PieceOpError,
-    state: PreparedPieceState,
+    partially: PieceOpErrorPartiallyProcessed,
+    state: POEPU,
   },
 }
-display_as_debug!{ErrorSignaledViaUpdate}
+display_as_debug!{ErrorSignaledViaUpdate<T>, <T:Debug>}
+
+#[derive(Error,Debug,Serialize,Copy,Clone,Eq,PartialEq)]
+pub enum PieceOpErrorPartiallyProcessed {
+  Unprocessed,
+  Partially,
+}
+display_as_debug!{PieceOpErrorPartiallyProcessed}
 
 #[derive(Error,Debug,Serialize,Copy,Clone)]
 pub enum PieceOpError {

diff --git a/src/global.rs b/src/global.rs
index 3606d63172af0203963ddeed6ccdd7383bc8108e..9f9518904cb5f39bb36a510f18fc2c78246f4b62 100644 (file)
--- a/src/global.rs
+++ b/src/global.rs
@@ -10,7 +10,9 @@ use slotmap::dense as sm;
 use std::sync::PoisonError;
 
 type ME = MgmtError;
-type ESU = ErrorSignaledViaUpdate;
+type ESU<POEPU> = ErrorSignaledViaUpdate<POEPU>;
+
+#[allow(non_camel_case_types)] type PUE_P = PreparedUpdateEntry_Piece;
 
 // ---------- newtypes and type aliases ----------
 
@@ -582,7 +584,7 @@ impl<'ig> InstanceGuard<'ig> {
 
   pub fn remove_clients(&mut self,
                         players: &HashSet<PlayerId>,
-                        signal: ErrorSignaledViaUpdate) {
+                        signal: ErrorSignaledViaUpdate<PUE_P>) {
     let mut clients_to_remove = HashSet::new();
     self.clients.retain(|k,v| {
       let remove = players.contains(&v.player);
@@ -597,7 +599,6 @@ impl<'ig> InstanceGuard<'ig> {
           gen,
           when: Instant::now(),
           us: vec![ PreparedUpdateEntry::Error(
-            None,
             signal.clone(),
           )],
         });

diff --git a/src/imports.rs b/src/imports.rs
index 7fe307b388cb5c3cafcdff50258e93561069a3a3..72af04110d437eea78793f0ae9ae5705069e9e7a 100644 (file)
--- a/src/imports.rs
+++ b/src/imports.rs
@@ -117,3 +117,4 @@ display_as_debug!(Impossible);
 
 pub type AE = anyhow::Error;
 pub type OE = OnlineError;
+pub type POEPP = PieceOpErrorPartiallyProcessed;

diff --git a/src/updates.rs b/src/updates.rs
index 9e94527dd48f502a89b92ca4767905559bf8f451..f5a669bac8f3d36df007a0259bef0c8557ce9d4d 100644 (file)
--- a/src/updates.rs
+++ b/src/updates.rs
@@ -7,6 +7,8 @@
 use crate::imports::*;
 
 type PUE = PreparedUpdateEntry;
+type ESVU<POEPU> = ErrorSignaledViaUpdate<POEPU>;
+
 #[allow(non_camel_case_types)] type PUE_P = PreparedUpdateEntry_Piece;
 #[allow(non_camel_case_types)] type TUE_P<'u> = TransmitUpdateEntry_Piece<'u>;
 
@@ -59,7 +61,7 @@ pub enum PreparedUpdateEntry {
     new_info_pane: Arc<Html>,
   },
   Log(Arc<CommittedLogEntry>),
-  Error(Option<ClientId> /* none: all */, ErrorSignaledViaUpdate),
+  Error(ErrorSignaledViaUpdate<PUE_P>),
 }
 
 #[allow(non_camel_case_types)]
@@ -138,7 +140,7 @@ enum TransmitUpdateEntry<'u> {
   SetLinks(Html),
   #[serde(serialize_with="serialize_logentry")]
   Log(TransmitUpdateLogEntry<'u>),
-  Error(&'u ErrorSignaledViaUpdate),
+  Error(ErrorSignaledViaUpdate<TUE_P<'u>>),
 }
 
 type TransmitUpdateLogEntry<'u> = (&'u Timezone, &'u CommittedLogEntry);
@@ -251,8 +253,13 @@ impl PreparedUpdateEntry {
           |(_k,v)| Some(50 + v.as_ref()?.len())
         ).sum::<usize>() + 50
       }
-      SetTableSize(_) |
-      Error(_,_) => {
+      Error(ESVU::PieceOpError { state, .. }) => {
+        100 + state.json_len()
+      }
+      Error(ESVU::InternalError) |
+      Error(ESVU::PlayerRemoved) |
+      Error(ESVU::TokenRevoked) |
+      SetTableSize(_) => {
         100
       }
     }
@@ -386,41 +393,37 @@ impl<'r> PrepareUpdatesBuffer<'r> {
     })
   }
 
-  fn new_for_error(ig: &'r mut Instance) -> Self {
-    Self::new(ig, None, Some(1))
-  }
   pub fn piece_report_error(ig: &mut Instance,
                             error: PieceOpError, piece: PieceId,
-                            logents: Vec<LogEntry>, client: ClientId,
+                            logents: Vec<LogEntry>,
+                            partially: PieceOpErrorPartiallyProcessed,
+                            client: ClientId, cseq: ClientSequence,
                             lens: &dyn Lens) -> Result<(),OE> {
-    let mut buf = PrepareUpdatesBuffer::new_for_error(ig);
-    let update = buf.piece_update_fallible(
-      piece, PieceUpdateOp::Modify(()), lens
+    let by_client = (WRC::Unpredictable, client, cseq);
+    let mut buf = PrepareUpdatesBuffer::new(ig, Some(by_client), None);
+    let state = buf.piece_update_fallible(
+      piece, PieceUpdateOp::Modify(()), lens, |pc, gen, _by_client| {
+        match partially {
+          POEPP::Unprocessed => { }
+          POEPP::Partially => { pc.gen = gen; pc.lastclient = default(); }
+        }
+      }
     )?;
-    let update = match update {
-      PUE_P {
-        piece,
-        op: PieceUpdateOp::Modify(state),
-        ..
-      } => {
-        PreparedUpdateEntry::Error(
-          Some(client),
-          ErrorSignaledViaUpdate::PieceOpError {
-            piece, error, state,
-          },
-        )
-      },
-      _ => panic!(),
-    };
+    let update = PUE::Error(ESVU::PieceOpError {
+      error, state, partially
+    });
     buf.us.push(update);
     buf.log_updates(logents);
     Ok(())
   }
 
   #[throws(InternalError)]
-  fn piece_update_fallible(&mut self, piece: PieceId,
-                           update: PieceUpdateOp<(),()>,
-                           lens: &dyn Lens) -> PreparedUpdateEntry_Piece {
+  fn piece_update_fallible<GUF>(&mut self, piece: PieceId,
+                                update: PieceUpdateOp<(),()>,
+                                lens: &dyn Lens,
+                                gen_update: GUF) -> PreparedUpdateEntry_Piece
+    where GUF: FnOnce(&mut PieceState, Generation, &IsResponseToClientOp)
+  {
     type WRC = WhatResponseToClientOp;
 
     let gen = self.gen();
@@ -433,16 +436,7 @@ impl<'r> PrepareUpdatesBuffer<'r> {
       (Ok(pc), Some(p)) => {
         gs.max_z.update_max(&pc.zlevel.z);
 
-        if let Some(new_lastclient) = match self.by_client {
-          Some((WRC::Predictable,tclient,_)) |
-          Some((WRC::UpdateSvg,  tclient,_))
-            => if tclient == pc.lastclient { None } else { Some(tclient) }
-          _ => Some(Default::default()),
-        } {
-          pc.gen_before_lastclient = pc.gen;
-          pc.lastclient = new_lastclient;
-        }
-        pc.gen = gen;
+        gen_update(pc, gen, &self.by_client);
         let pri_for_all = lens.svg_pri(piece,pc,Default::default());
 
         let update = update.try_map(
@@ -475,13 +469,30 @@ impl<'r> PrepareUpdatesBuffer<'r> {
     // Caller needs us to be infallible since it is too late by
     // this point to back out a game state change.
 
-    let update = self.piece_update_fallible(piece, update, lens)
+    let update = self.piece_update_fallible(
+      piece, update, lens,
+      |pc, gen, by_client|
+    {
+      match *by_client {
+        Some((WRC::Predictable,tclient,_)) |
+        Some((WRC::UpdateSvg,  tclient,_)) => {
+          if tclient != pc.lastclient {
+            pc.gen_before_lastclient = pc.gen;
+            pc.lastclient = tclient;
+          }
+        }
+        Some((WRC::Unpredictable, _,_)) |
+          None => {
+            pc.lastclient = default();
+          }
+      }
+      pc.gen = gen;
+    })
       .map(|update| PUE::Piece(update))
       .unwrap_or_else(|e| {
         error!("piece update error! piece={:?} lens={:?} error={:?}",
                piece, &lens, &e);
-        PreparedUpdateEntry::Error(None,
-                                   ErrorSignaledViaUpdate::InternalError)
+        PreparedUpdateEntry::Error(ErrorSignaledViaUpdate::InternalError)
       });
     self.us.push(update);
   }
@@ -534,7 +545,7 @@ type WRC = WhatResponseToClientOp;
 impl PreparedUpdate {
   pub fn for_transmit<'u>(&'u self, tz: &'u Timezone, dest : ClientId)
                       -> TransmitUpdate<'u> {
-    type ESVU = ErrorSignaledViaUpdate;
+    type ESVU<T> = ErrorSignaledViaUpdate<T>;
     type PUE = PreparedUpdateEntry;
     type TUE<'u> = TransmitUpdateEntry<'u>;
     let mut ents = vec![];
@@ -597,14 +608,25 @@ impl PreparedUpdate {
         &PUE::RemovePlayer { player, ref new_info_pane } => {
           TUE::RemovePlayer { player, new_info_pane }
         }
-        PUE::Error(c, e) => {
-          if *c == None || *c == Some(dest) {
-            TUE::Error(e)
-          } else if let &ESVU::PieceOpError { piece, ref state, .. } = e {
-            let op = PieceUpdateOp::Modify(state);
-            TUE::Piece(TUE_P { piece, op })
-          } else {
-            continue
+        PUE::Error(e) => {
+          match *e {
+            ESVU::InternalError => TUE::Error(ESVU::InternalError),
+            ESVU::PlayerRemoved => TUE::Error(ESVU::PlayerRemoved),
+            ESVU::TokenRevoked  => TUE::Error(ESVU::TokenRevoked),
+            ESVU::PieceOpError { error, partially, ref state } => {
+              let c = state.by_client.as_ref().map(|(_,c,_)| *c);
+              if c == None || c == Some(dest) {
+                let state = pue_piece_to_tue_p(state);
+                TUE::Error(
+                  ESVU::PieceOpError { error, partially, state }
+                )
+              } else {
+                match partially {
+                  POEPP::Unprocessed => continue,
+                  POEPP::Partially => pue_piece_to_tue(&state, dest),
+                }
+              }
+            }
           }
         }
         PUE::SetLinks(links) => {

diff --git a/templates/script.ts b/templates/script.ts
index 32641652daca9bed7786a076110f0c007179b272..22b40df32db764f628966dd4994ec12392414a45 100644 (file)
--- a/templates/script.ts
+++ b/templates/script.ts
@@ -1010,8 +1010,12 @@ function zoom_activate() {
 
 // ----- test counter, startup -----
 
-messages.Piece = <MessageHandler>function
-(j: { piece: PieceId, op: Object }) {
+type TransmitUpdateEntry_Piece = {
+  piece: PieceId,
+  op: Object,
+};
+
+function handle_piece_update(j: TransmitUpdateEntry_Piece) {
   console.log('PIECE UPDATE ',j)
   var piece = j.piece;
   var m = j.op as { [k: string]: Object };
@@ -1020,6 +1024,8 @@ messages.Piece = <MessageHandler>function
   pieceops[k](piece,p, m[k]);
 };
 
+messages.Piece = <MessageHandler>handle_piece_update;
+
 type PieceStateMessage = {
   svg: string,
   held: PlayerId,
@@ -1192,18 +1198,18 @@ messages.Error = <MessageHandler>function
 }
 
 type PieceOpError = {
-  piece: PieceId,
   error: string,
-  state: PieceStateMessage,
+  state: TransmitUpdateEntry_Piece,
 };
 
 update_error_handlers.PieceOpError = <MessageHandler>function
 (m: PieceOpError) {
-  let p = pieces[m.piece];
+  let piece = m.state.piece;
+  let p = pieces[piece];
   console.log('ERROR UPDATE PIECE ', m, p);
   if (p == null) return;
-  let conflict_expected = piece_error_handlers[m.error](m.piece, p, m);
-  piece_modify(m.piece, p, m.state, conflict_expected);
+  let conflict_expected = piece_error_handlers[m.error](piece, p, m);
+  handle_piece_update(m.state);
 }
 
 function piece_checkconflict_nrda(piece: PieceId, p: PieceInfo,