From 8a86b9ea7ef25c87c8450f7ec5036e26b6e763dd Mon Sep 17 00:00:00 2001 From: Ian Jackson Date: Sat, 30 Jan 2021 00:36:53 +0000 Subject: [PATCH] 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 --- daemon/api.rs | 4 +- src/error.rs | 19 ++++--- src/global.rs | 7 +-- src/imports.rs | 1 + src/updates.rs | 124 ++++++++++++++++++++++++++------------------ templates/script.ts | 20 ++++--- 6 files changed, 106 insertions(+), 69 deletions(-) diff --git a/daemon/api.rs b/daemon/api.rs index c0ec99a1..97c3779b 100644 --- a/daemon/api.rs +++ b/daemon/api.rs @@ -144,14 +144,14 @@ fn api_piece_op(form : Json>) 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 04e4978f..8fbded1e 100644 --- a/src/error.rs +++ b/src/error.rs @@ -96,17 +96,24 @@ impl From for ApiPieceOpError { } #[derive(Error,Debug,Serialize,Clone)] -pub enum ErrorSignaledViaUpdate { +pub enum ErrorSignaledViaUpdate { 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, } + +#[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 3606d631..9f951890 100644 --- 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 = ErrorSignaledViaUpdate; + +#[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, - signal: ErrorSignaledViaUpdate) { + signal: ErrorSignaledViaUpdate) { 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 7fe307b3..72af0411 100644 --- 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 9e94527d..f5a669ba 100644 --- a/src/updates.rs +++ b/src/updates.rs @@ -7,6 +7,8 @@ use crate::imports::*; type PUE = PreparedUpdateEntry; +type ESVU = ErrorSignaledViaUpdate; + #[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, }, Log(Arc), - Error(Option /* none: all */, ErrorSignaledViaUpdate), + Error(ErrorSignaledViaUpdate), } #[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>), } type TransmitUpdateLogEntry<'u> = (&'u Timezone, &'u CommittedLogEntry); @@ -251,8 +253,13 @@ impl PreparedUpdateEntry { |(_k,v)| Some(50 + v.as_ref()?.len()) ).sum::() + 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, client: ClientId, + logents: Vec, + 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(&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 = ErrorSignaledViaUpdate; 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 32641652..22b40df3 100644 --- a/templates/script.ts +++ b/templates/script.ts @@ -1010,8 +1010,12 @@ function zoom_activate() { // ----- test counter, startup ----- -messages.Piece = 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 = function pieceops[k](piece,p, m[k]); }; +messages.Piece = handle_piece_update; + type PieceStateMessage = { svg: string, held: PlayerId, @@ -1192,18 +1198,18 @@ messages.Error = function } type PieceOpError = { - piece: PieceId, error: string, - state: PieceStateMessage, + state: TransmitUpdateEntry_Piece, }; update_error_handlers.PieceOpError = 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, -- 2.30.2