From 734b832c0061196517d346e212df213926572fa0 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Mon, 30 Oct 2023 14:16:50 +0800 Subject: [PATCH] Fix checkout event (#126) * tests: add checkout err tests * fix: checkout event err when create child --- .vscode/settings.json | 2 +- crates/loro-internal/src/arena.rs | 68 +- .../src/container/richtext/richtext_state.rs | 2 +- crates/loro-internal/src/diff_calc.rs | 186 ++++- crates/loro-internal/src/diff_calc/tree.rs | 10 +- crates/loro-internal/src/event.rs | 11 + .../src/fuzz/recursive_refactored.rs | 649 ++++++++++++++---- crates/loro-internal/src/state.rs | 24 +- crates/loro-internal/src/state/list_state.rs | 2 +- .../loro-internal/src/state/richtext_state.rs | 4 - crates/loro-internal/src/txn.rs | 87 ++- crates/loro-internal/tests/test.rs | 57 +- 12 files changed, 857 insertions(+), 245 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index 29a72e94..70fb0235 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -23,7 +23,7 @@ "yspan" ], "rust-analyzer.runnableEnv": { - "RUST_BACKTRACE": "1", + "RUST_BACKTRACE": "full", "DEBUG": "*" }, "rust-analyzer.cargo.features": ["test_utils"], diff --git a/crates/loro-internal/src/arena.rs b/crates/loro-internal/src/arena.rs index 0ebd2ceb..176a97b3 100644 --- a/crates/loro-internal/src/arena.rs +++ b/crates/loro-internal/src/arena.rs @@ -30,6 +30,8 @@ struct InnerSharedArena { // The locks should not be exposed outside this file. // It might be better to use RwLock in the future container_idx_to_id: Mutex>, + // 0 stands for unknown, 1 stands for root containers + depth: Mutex>, container_id_to_idx: Mutex>, /// The parent of each container. parents: Mutex>>, @@ -55,6 +57,7 @@ pub struct StrAllocResult { pub(crate) struct OpConverter<'a> { container_idx_to_id: MutexGuard<'a, Vec>, container_id_to_idx: MutexGuard<'a, FxHashMap>, + container_idx_depth: MutexGuard<'a, Vec>, str: MutexGuard<'a, StrArena>, values: MutexGuard<'a, Vec>, root_c_idx: MutexGuard<'a, Vec>, @@ -83,8 +86,10 @@ impl<'a> OpConverter<'a> { if id.is_root() { self.root_c_idx.push(idx); self.parents.insert(idx, None); + self.container_idx_depth.push(1); + } else { + self.container_idx_depth.push(0); } - idx }; @@ -164,6 +169,7 @@ impl<'a> OpConverter<'a> { let container_idx_to_id = &mut self.container_idx_to_id; let idx = container_idx_to_id.len(); container_idx_to_id.push(meta_container_id.clone()); + self.container_idx_depth.push(0); let idx = ContainerIdx::from_index_and_type( idx as u32, meta_container_id.container_type(), @@ -198,6 +204,9 @@ impl SharedArena { if id.is_root() { self.inner.root_c_idx.lock().unwrap().push(idx); self.inner.parents.lock().unwrap().insert(idx, None); + self.inner.depth.lock().unwrap().push(1); + } else { + self.inner.depth.lock().unwrap().push(0); } idx } @@ -222,6 +231,12 @@ impl SharedArena { lock.get(id.to_index() as usize).cloned() } + #[inline] + pub fn with_idx_to_id(&self, f: impl FnOnce(&Vec) -> R) -> R { + let lock = self.inner.container_idx_to_id.lock().unwrap(); + f(&lock) + } + pub fn alloc_str(&self, str: &str) -> StrAllocResult { let mut text_lock = self.inner.str.lock().unwrap(); _alloc_str(&mut text_lock, str) @@ -261,7 +276,22 @@ impl SharedArena { #[inline] pub fn set_parent(&self, child: ContainerIdx, parent: Option) { - self.inner.parents.lock().unwrap().insert(child, parent); + let parents = &mut self.inner.parents.lock().unwrap(); + parents.insert(child, parent); + let mut depth = self.inner.depth.lock().unwrap(); + + match parent { + Some(p) => { + if let Some(d) = get_depth(p, &mut depth, parents) { + depth[child.to_index() as usize] = d + 1; + } else { + depth[child.to_index() as usize] = 0; + } + } + None => { + depth[child.to_index() as usize] = 1; + } + } } pub fn log_hierarchy(&self) { @@ -339,6 +369,7 @@ impl SharedArena { let mut op_converter = OpConverter { container_idx_to_id: self.inner.container_idx_to_id.lock().unwrap(), container_id_to_idx: self.inner.container_id_to_idx.lock().unwrap(), + container_idx_depth: self.inner.depth.lock().unwrap(), str: self.inner.str.lock().unwrap(), values: self.inner.values.lock().unwrap(), root_c_idx: self.inner.root_c_idx.lock().unwrap(), @@ -479,6 +510,14 @@ impl SharedArena { pub fn root_containers(&self) -> Vec { self.inner.root_c_idx.lock().unwrap().clone() } + + pub(crate) fn get_depth(&self, container: ContainerIdx) -> Option { + get_depth( + container, + &mut self.inner.depth.lock().unwrap(), + &self.inner.parents.lock().unwrap(), + ) + } } fn _alloc_values( @@ -515,3 +554,28 @@ fn _slice_str(range: Range, s: &mut StrArena) -> String { ans.push_str(s.slice_str_by_unicode(range)); ans } + +fn get_depth( + target: ContainerIdx, + depth: &mut Vec, + parents: &FxHashMap>, +) -> Option { + let mut d = depth[target.to_index() as usize]; + if d != 0 { + return Some(d); + } + + let parent = parents.get(&target)?; + match parent { + Some(p) => { + d = get_depth(*p, depth, parents)? + 1; + depth[target.to_index() as usize] = d; + } + None => { + depth[target.to_index() as usize] = 1; + d = 1; + } + } + + Some(d) +} diff --git a/crates/loro-internal/src/container/richtext/richtext_state.rs b/crates/loro-internal/src/container/richtext/richtext_state.rs index 99987e0c..7ce749b6 100644 --- a/crates/loro-internal/src/container/richtext/richtext_state.rs +++ b/crates/loro-internal/src/container/richtext/richtext_state.rs @@ -1310,7 +1310,7 @@ impl RichtextState { "pos: {}, len: {}, self.len(): {}", pos, len, - self.len_entity() + self.to_string() ); // PERF: may use cache to speed up self.cursor_cache.invalidate(); diff --git a/crates/loro-internal/src/diff_calc.rs b/crates/loro-internal/src/diff_calc.rs index 6ba3af2e..cf041250 100644 --- a/crates/loro-internal/src/diff_calc.rs +++ b/crates/loro-internal/src/diff_calc.rs @@ -1,11 +1,12 @@ use std::sync::Arc; pub(super) mod tree; +use itertools::Itertools; pub(super) use tree::TreeDiffCache; use enum_dispatch::enum_dispatch; use fxhash::{FxHashMap, FxHashSet}; -use loro_common::{HasIdSpan, PeerID, ID}; +use loro_common::{ContainerID, HasIdSpan, LoroValue, PeerID, ID}; use crate::{ change::Lamport, @@ -38,7 +39,10 @@ use super::{event::InternalContainerDiff, oplog::OpLog}; /// TODO: persist diffCalculator and skip processed version #[derive(Debug, Default)] pub struct DiffCalculator { - calculators: FxHashMap, + /// ContainerIdx -> (depth, calculator) + /// + /// if depth == u16::MAX, we need to calculate it again + calculators: FxHashMap, last_vv: VersionVector, has_all: bool, } @@ -124,20 +128,26 @@ impl DiffCalculator { let mut visited = FxHashSet::default(); for op in change.ops.iter() { - let calculator = + let depth = oplog.arena.get_depth(op.container).unwrap_or(u16::MAX); + let (_, calculator) = self.calculators.entry(op.container).or_insert_with(|| { match op.container.get_type() { - crate::ContainerType::Text => ContainerDiffCalculator::Richtext( - RichtextDiffCalculator::default(), + crate::ContainerType::Text => ( + depth, + ContainerDiffCalculator::Richtext( + RichtextDiffCalculator::default(), + ), + ), + crate::ContainerType::Map => ( + depth, + ContainerDiffCalculator::Map(MapDiffCalculator::new()), + ), + crate::ContainerType::List => ( + depth, + ContainerDiffCalculator::List(ListDiffCalculator::default()), ), - crate::ContainerType::Map => { - ContainerDiffCalculator::Map(MapDiffCalculator::new()) - } - crate::ContainerType::List => { - ContainerDiffCalculator::List(ListDiffCalculator::default()) - } crate::ContainerType::Tree => { - ContainerDiffCalculator::Tree(TreeDiffCalculator::default()) + (depth, ContainerDiffCalculator::Tree(TreeDiffCalculator)) } } }); @@ -160,7 +170,7 @@ impl DiffCalculator { } } } - for (_, calculator) in self.calculators.iter_mut() { + for (_, (_, calculator)) in self.calculators.iter_mut() { calculator.stop_tracking(oplog, after); } @@ -181,25 +191,115 @@ impl DiffCalculator { None } }; - let mut diffs = Vec::with_capacity(self.calculators.len()); - if let Some(set) = affected_set { + + // Because we need to get correct `reset` value that indicates container is created during this round of diff calc, + // we need to iterate from parents to children. i.e. from smaller depth to larger depth. + let mut new_containers: FxHashSet = FxHashSet::default(); + let empty_vv: VersionVector = Default::default(); + let mut all: Vec<(u16, ContainerIdx)> = if let Some(set) = affected_set { // only visit the affected containers - for idx in set { - let calc = self.calculators.get_mut(&idx).unwrap(); - diffs.push(InternalContainerDiff { - idx, - diff: calc.calculate_diff(oplog, before, after).into(), - }); - } + set.into_iter() + .map(|x| { + let (depth, _) = self.calculators.get_mut(&x).unwrap(); + (*depth, x) + }) + .collect() } else { - for (&idx, calculator) in self.calculators.iter_mut() { - diffs.push(InternalContainerDiff { - idx, - diff: calculator.calculate_diff(oplog, before, after).into(), + self.calculators + .iter_mut() + .map(|(x, (depth, _))| (*depth, *x)) + .collect() + }; + + let mut are_rest_containers_deleted = false; + let mut ans = FxHashMap::default(); + while !all.is_empty() { + // sort by depth and lamport, ensure we iterate from top to bottom + all.sort_by_key(|x| x.0); + debug_log::debug_dbg!(&all); + let len = all.len(); + for (_, idx) in std::mem::take(&mut all) { + if ans.contains_key(&idx) { + continue; + } + + let (depth, calc) = self.calculators.get_mut(&idx).unwrap(); + if *depth == u16::MAX && !are_rest_containers_deleted { + if let Some(d) = oplog.arena.get_depth(idx) { + *depth = d; + } + + all.push((*depth, idx)); + continue; + } + + let (from, reset) = if new_containers.remove(&oplog.arena.idx_to_id(idx).unwrap()) { + // if the container is new, we need to calculate the diff from the beginning + (&empty_vv, true) + } else { + (before, false) + }; + + let diff = calc.calculate_diff(oplog, from, after, |c| { + new_containers.insert(c.clone()); + let child_idx = oplog.arena.register_container(c); + oplog.arena.set_parent(child_idx, Some(idx)); }); + if !diff.is_empty() || reset { + ans.insert( + idx, + InternalContainerDiff { + idx, + reset, + is_container_deleted: are_rest_containers_deleted, + diff: diff.into(), + }, + ); + } + } + + debug_log::debug_dbg!(&new_containers); + // reset left new_containers + while !new_containers.is_empty() { + for id in std::mem::take(&mut new_containers) { + let Some(idx) = oplog.arena.id_to_idx(&id) else { + continue; + }; + let Some((_, calc)) = self.calculators.get_mut(&idx) else { + continue; + }; + let diff = calc.calculate_diff(oplog, &empty_vv, after, |c| { + new_containers.insert(c.clone()); + let child_idx = oplog.arena.register_container(c); + oplog.arena.set_parent(child_idx, Some(idx)); + }); + // this can override the previous diff with `reset = false` + // otherwise, the diff event will be incorrect + ans.insert( + idx, + InternalContainerDiff { + idx, + reset: true, + is_container_deleted: false, + diff: diff.into(), + }, + ); + } + } + + if len == all.len() { + debug_log::debug_log!("Container might be deleted"); + debug_log::debug_dbg!(&all); + for (_, idx) in all.iter() { + debug_log::debug_dbg!(oplog.arena.get_container_id(*idx)); + } + // we still emit the event of deleted container + are_rest_containers_deleted = true; } } - diffs + + debug_log::debug_dbg!(&ans); + ans.into_iter().map(|x| x.1).collect_vec() } } @@ -225,6 +325,7 @@ pub trait DiffCalculatorTrait { oplog: &OpLog, from: &crate::VersionVector, to: &crate::VersionVector, + on_new_container: impl FnMut(&ContainerID), ) -> InternalDiff; } @@ -278,6 +379,7 @@ impl DiffCalculatorTrait for MapDiffCalculator { oplog: &super::oplog::OpLog, from: &crate::VersionVector, to: &crate::VersionVector, + mut on_new_container: impl FnMut(&ContainerID), ) -> InternalDiff { let mut changed = Vec::new(); for (k, g) in self.grouped.iter_mut() { @@ -299,6 +401,10 @@ impl DiffCalculatorTrait for MapDiffCalculator { let value = value .map(|v| { let value = v.value.and_then(|v| oplog.arena.get_value(v as usize)); + if let Some(LoroValue::Container(c)) = &value { + on_new_container(c); + } + MapValue { counter: v.counter, value, @@ -412,11 +518,27 @@ impl DiffCalculatorTrait for ListDiffCalculator { fn calculate_diff( &mut self, - _oplog: &OpLog, + oplog: &OpLog, from: &crate::VersionVector, to: &crate::VersionVector, + mut on_new_container: impl FnMut(&ContainerID), ) -> InternalDiff { - InternalDiff::SeqRaw(self.tracker.diff(from, to)) + let ans = self.tracker.diff(from, to); + // PERF: We may simplify list to avoid these getting + for v in ans.iter() { + if let crate::delta::DeltaItem::Insert { value, meta: _ } = &v { + for range in &value.0 { + for i in range.0.clone() { + let v = oplog.arena.get_value(i as usize); + if let Some(LoroValue::Container(c)) = &v { + on_new_container(c); + } + } + } + } + } + + InternalDiff::SeqRaw(ans) } } @@ -518,6 +640,7 @@ impl DiffCalculatorTrait for RichtextDiffCalculator { oplog: &OpLog, from: &crate::VersionVector, to: &crate::VersionVector, + _: impl FnMut(&ContainerID), ) -> InternalDiff { let mut delta = Delta::new(); for item in self.tracker.diff(from, to) { @@ -549,6 +672,8 @@ impl DiffCalculatorTrait for RichtextDiffCalculator { } } + // FIXME: handle new containers when inserting richtext style like comments + // debug_log::debug_dbg!(&delta, from, to); // debug_log::debug_dbg!(&self.tracker); InternalDiff::RichtextRaw(delta) @@ -607,6 +732,7 @@ impl DiffCalculatorTrait for TreeDiffCalculator { oplog: &OpLog, from: &crate::VersionVector, to: &crate::VersionVector, + _: impl FnMut(&ContainerID), ) -> InternalDiff { debug_log::debug_log!("from {:?} to {:?}", from, to); let mut merged_vv = from.clone(); @@ -633,6 +759,8 @@ impl DiffCalculatorTrait for TreeDiffCalculator { lca_min_lamport, (from_min_lamport, from_max_lamport), ); + + // FIXME: inserting new containers debug_log::debug_log!("\ndiff {:?}", diff); InternalDiff::Tree(diff) diff --git a/crates/loro-internal/src/diff_calc/tree.rs b/crates/loro-internal/src/diff_calc/tree.rs index 0851719d..a4723851 100644 --- a/crates/loro-internal/src/diff_calc/tree.rs +++ b/crates/loro-internal/src/diff_calc/tree.rs @@ -87,10 +87,7 @@ impl TreeDiffCache { // Because importing the local op must not cause circular references, it has been checked. pub(crate) fn add_node_uncheck(&mut self, node: MoveLamportAndID) { if !self.all_version.includes_id(node.id) { - self.cache - .entry(node.target) - .or_insert_with(Default::default) - .insert(node); + self.cache.entry(node.target).or_default().insert(node); // assert len == 1 self.current_version.set_last(node.id); self.all_version.set_last(node.id); @@ -178,10 +175,7 @@ impl TreeDiffCache { ans = false; } node.effected = ans; - self.cache - .entry(node.target) - .or_insert_with(Default::default) - .insert(node); + self.cache.entry(node.target).or_default().insert(node); self.current_version.set_last(node.id); ans } diff --git a/crates/loro-internal/src/event.rs b/crates/loro-internal/src/event.rs index 9417da3b..3171d5d5 100644 --- a/crates/loro-internal/src/event.rs +++ b/crates/loro-internal/src/event.rs @@ -49,6 +49,8 @@ pub struct DocDiff { #[derive(Debug, Clone)] pub(crate) struct InternalContainerDiff { pub(crate) idx: ContainerIdx, + pub(crate) reset: bool, + pub(crate) is_container_deleted: bool, pub(crate) diff: DiffVariant, } @@ -177,6 +179,15 @@ pub enum Diff { } impl InternalDiff { + pub(crate) fn is_empty(&self) -> bool { + match self { + InternalDiff::SeqRaw(s) => s.is_empty(), + InternalDiff::RichtextRaw(t) => t.is_empty(), + InternalDiff::Map(m) => m.updated.is_empty(), + InternalDiff::Tree(t) => t.diff.is_empty(), + } + } + pub(crate) fn compose(self, diff: InternalDiff) -> Result { // PERF: avoid clone match (self, diff) { diff --git a/crates/loro-internal/src/fuzz/recursive_refactored.rs b/crates/loro-internal/src/fuzz/recursive_refactored.rs index a6a9bcc3..a4c70007 100644 --- a/crates/loro-internal/src/fuzz/recursive_refactored.rs +++ b/crates/loro-internal/src/fuzz/recursive_refactored.rs @@ -89,11 +89,13 @@ impl Actor { let root_value = Arc::clone(&actor.value_tracker); actor.loro.subscribe_deep(Arc::new(move |event| { let mut root_value = root_value.lock().unwrap(); - // debug_log::debug_dbg!(&event); + debug_log::debug_dbg!(&event.container); + debug_log::debug_dbg!(&root_value); root_value.apply( &event.container.path.iter().map(|x| x.1.clone()).collect(), &[event.container.diff.clone()], ); + debug_log::debug_dbg!(&root_value); })); let text = Arc::clone(&actor.text_tracker); @@ -670,6 +672,7 @@ fn check_eq(a_actor: &mut Actor, b_actor: &mut Actor) { debug_log::debug_log!("{}", a_result.to_json_pretty()); assert_eq!(&a_result, &b_doc.get_state_deep_value()); assert_value_eq(&a_result, &a_actor.value_tracker.lock().unwrap()); + assert_value_eq(&a_result, &b_actor.value_tracker.lock().unwrap()); let a = a_doc.get_text("text"); let value_a = a.get_value(); @@ -700,6 +703,8 @@ fn check_synced(sites: &mut [Actor]) { let (a, b) = array_mut_ref!(sites, [i, j]); let a_doc = &mut a.loro; let b_doc = &mut b.loro; + debug_log::debug_dbg!(a_doc.get_deep_value_with_id()); + debug_log::debug_dbg!(b_doc.get_deep_value_with_id()); if (i + j) % 2 == 0 { debug_log::group!("Updates {} to {}", j, i); a_doc.import(&b_doc.export_from(&a_doc.oplog_vv())).unwrap(); @@ -730,11 +735,18 @@ fn check_synced(sites: &mut [Actor]) { fn check_history(actor: &mut Actor) { assert!(!actor.history.is_empty()); - for (c, (f, v)) in actor.history.iter().enumerate() { + for (_, (f, v)) in actor.history.iter().enumerate() { let f = Frontiers::from(f); + debug_log::group!( + "Checkout from {:?} to {:?}", + &actor.loro.state_frontiers(), + &f + ); actor.loro.checkout(&f).unwrap(); let actual = actor.loro.get_deep_value(); - assert_eq!(v, &actual, "Version mismatched at {:?}, cnt={}", f, c); + assert_value_eq(v, &actual); + assert_value_eq(v, &actor.value_tracker.lock().unwrap()); + debug_log::group_end!(); } } @@ -794,6 +806,7 @@ mod failed_tests { use super::test_multi_sites; use super::Action; use super::Action::*; + use super::ContainerType as C; use super::FuzzValue::*; use arbtest::arbitrary::{self, Unstructured}; @@ -960,89 +973,88 @@ mod failed_tests { ); } - // FIXME: Need to emit events involving child container re-creation - // #[test] - // fn fuzz_2() { - // test_multi_sites( - // 5, - // &mut [ - // Map { - // site: 0, - // container_idx: 0, - // key: 0, - // value: I32(1616928864), - // }, - // List { - // site: 96, - // container_idx: 96, - // key: 96, - // value: I32(1616928864), - // }, - // List { - // site: 96, - // container_idx: 96, - // key: 96, - // value: I32(1616928864), - // }, - // List { - // site: 96, - // container_idx: 96, - // key: 96, - // value: Container(C::Text), - // }, - // List { - // site: 55, - // container_idx: 55, - // key: 55, - // value: Null, - // }, - // SyncAll, - // List { - // site: 55, - // container_idx: 64, - // key: 53, - // value: Null, - // }, - // List { - // site: 56, - // container_idx: 56, - // key: 56, - // value: Container(C::Text), - // }, - // List { - // site: 0, - // container_idx: 0, - // key: 0, - // value: Null, - // }, - // List { - // site: 64, - // container_idx: 64, - // key: 64, - // value: I32(1616928864), - // }, - // List { - // site: 96, - // container_idx: 96, - // key: 96, - // value: I32(1616928864), - // }, - // List { - // site: 96, - // container_idx: 96, - // key: 255, - // value: I32(7), - // }, - // Text { - // site: 97, - // container_idx: 225, - // pos: 97, - // value: 24929, - // is_del: false, - // }, - // ], - // ); - // } + #[test] + fn fuzz_2() { + test_multi_sites( + 5, + &mut [ + Map { + site: 0, + container_idx: 0, + key: 0, + value: I32(1616928864), + }, + List { + site: 96, + container_idx: 96, + key: 96, + value: I32(1616928864), + }, + List { + site: 96, + container_idx: 96, + key: 96, + value: I32(1616928864), + }, + List { + site: 96, + container_idx: 96, + key: 96, + value: Container(C::Text), + }, + List { + site: 55, + container_idx: 55, + key: 55, + value: Null, + }, + SyncAll, + List { + site: 55, + container_idx: 64, + key: 53, + value: Null, + }, + List { + site: 56, + container_idx: 56, + key: 56, + value: Container(C::Text), + }, + List { + site: 0, + container_idx: 0, + key: 0, + value: Null, + }, + List { + site: 64, + container_idx: 64, + key: 64, + value: I32(1616928864), + }, + List { + site: 96, + container_idx: 96, + key: 96, + value: I32(1616928864), + }, + List { + site: 96, + container_idx: 96, + key: 255, + value: I32(7), + }, + Text { + site: 97, + container_idx: 225, + pos: 97, + value: 24929, + is_del: false, + }, + ], + ); + } #[test] fn fuzz_3() { @@ -1545,66 +1557,65 @@ mod failed_tests { ); } - // FIXME: same as fuzz_2 - // #[test] - // fn cannot_skip_ops_from_deleted_container_due_to_this_case() { - // test_multi_sites( - // 5, - // &mut [ - // List { - // site: 1, - // container_idx: 0, - // key: 0, - // value: Container(C::List), - // }, - // Map { - // site: 0, - // container_idx: 0, - // key: 2, - // value: Container(C::List), - // }, - // SyncAll, - // Map { - // site: 0, - // container_idx: 0, - // key: 255, - // value: Container(C::List), - // }, - // SyncAll, - // Map { - // site: 0, - // container_idx: 0, - // key: 255, - // value: Container(C::List), - // }, - // List { - // site: 3, - // container_idx: 3, - // key: 0, - // value: Container(C::List), - // }, - // List { - // site: 1, - // container_idx: 3, - // key: 0, - // value: Container(C::List), - // }, - // SyncAll, - // List { - // site: 0, - // container_idx: 3, - // key: 0, - // value: Container(C::Map), - // }, - // List { - // site: 1, - // container_idx: 3, - // key: 1, - // value: Container(C::Map), - // }, - // ], - // ) - // } + #[test] + fn cannot_skip_ops_from_deleted_container_due_to_this_case() { + test_multi_sites( + 5, + &mut [ + List { + site: 1, + container_idx: 0, + key: 0, + value: Container(C::List), + }, + Map { + site: 0, + container_idx: 0, + key: 2, + value: Container(C::List), + }, + SyncAll, + Map { + site: 0, + container_idx: 0, + key: 255, + value: Container(C::List), + }, + SyncAll, + Map { + site: 0, + container_idx: 0, + key: 255, + value: Container(C::List), + }, + List { + site: 3, + container_idx: 3, + key: 0, + value: Container(C::List), + }, + List { + site: 1, + container_idx: 3, + key: 0, + value: Container(C::List), + }, + SyncAll, + List { + site: 0, + container_idx: 3, + key: 0, + value: Container(C::Map), + }, + List { + site: 1, + container_idx: 3, + key: 1, + value: Container(C::Map), + }, + ], + ) + } #[test] fn map_apply() { @@ -2370,7 +2381,351 @@ mod failed_tests { ) } - use super::ContainerType as C; + #[test] + fn fuzz_6_reset_children() { + test_multi_sites( + 5, + &mut [ + Map { + site: 3, + container_idx: 1, + key: 0, + value: Null, + }, + Map { + site: 1, + container_idx: 136, + key: 0, + value: Container(C::Text), + }, + Text { + site: 1, + container_idx: 5, + pos: 0, + value: 0, + is_del: false, + }, + ], + ) + } + + #[test] + fn fuzz_7_checkout() { + test_multi_sites( + 5, + &mut [ + Map { + site: 1, + container_idx: 0, + key: 0, + value: Container(C::Text), + }, + List { + site: 4, + container_idx: 0, + key: 0, + value: Container(C::List), + }, + Sync { from: 1, to: 4 }, + List { + site: 4, + container_idx: 1, + key: 0, + value: I32(1566399837), + }, + List { + site: 1, + container_idx: 0, + key: 0, + value: Null, + }, + ], + ) + } + + #[test] + fn fuzz_8_checkout() { + test_multi_sites( + 5, + &mut [ + Text { + site: 0, + container_idx: 0, + pos: 0, + value: 16639, + is_del: false, + }, + List { + site: 0, + container_idx: 0, + key: 0, + value: Container(C::Text), + }, + Map { + site: 0, + container_idx: 0, + key: 245, + value: Container(C::List), + }, + SyncAll, + List { + site: 2, + container_idx: 0, + key: 0, + value: Container(C::Map), + }, + List { + site: 1, + container_idx: 1, + key: 0, + value: Container(C::Tree), + }, + Text { + site: 1, + container_idx: 1, + pos: 0, + value: 47288, + is_del: false, + }, + List { + site: 0, + container_idx: 0, + key: 0, + value: Null, + }, + ], + ) + } + + #[test] + fn fuzz_9_checkout() { + test_multi_sites( + 5, + &mut [ + Map { + site: 0, + container_idx: 0, + key: 223, + value: Container(C::Map), + }, + List { + site: 213, + container_idx: 85, + key: 85, + value: Null, + }, + Map { + site: 0, + container_idx: 1, + key: 14, + value: Container(C::Text), + }, + Map { + site: 223, + container_idx: 255, + key: 126, + value: Container(C::Tree), + }, + Text { + site: 255, + container_idx: 255, + pos: 120, + value: 9215, + is_del: false, + }, + ], + ) + } + + #[test] + fn fuzz_10_checkout() { + test_multi_sites( + 5, + &mut [ + List { + site: 2, + container_idx: 0, + key: 0, + value: Container(C::Tree), + }, + Map { + site: 0, + container_idx: 0, + key: 0, + value: I32(-167), + }, + List { + site: 0, + container_idx: 0, + key: 0, + value: Container(C::Map), + }, + Sync { from: 1, to: 0 }, + List { + site: 0, + container_idx: 0, + key: 0, + value: Container(C::Tree), + }, + Map { + site: 0, + container_idx: 1, + key: 255, + value: Null, + }, + Map { + site: 0, + container_idx: 0, + key: 17, + value: Null, + }, + Map { + site: 0, + container_idx: 1, + key: 255, + value: Null, + }, + List { + site: 2, + container_idx: 0, + key: 0, + value: Container(C::List), + }, + List { + site: 4, + container_idx: 0, + key: 0, + value: Container(C::Map), + }, + Map { + site: 0, + container_idx: 1, + key: 191, + value: Container(C::List), + }, + SyncAll, + Map { + site: 0, + container_idx: 0, + key: 17, + value: Null, + }, + Map { + site: 0, + container_idx: 0, + key: 255, + value: Null, + }, + List { + site: 2, + container_idx: 1, + key: 0, + value: Container(C::List), + }, + List { + site: 4, + container_idx: 2, + key: 0, + value: Container(C::Map), + }, + SyncAll, + Map { + site: 2, + container_idx: 3, + key: 239, + value: I32(-1073741988), + }, + Map { + site: 0, + container_idx: 3, + key: 191, + value: Container(C::List), + }, + List { + site: 1, + container_idx: 1, + key: 0, + value: Container(C::Text), + }, + Map { + site: 0, + container_idx: 0, + key: 17, + value: Null, + }, + Map { + site: 0, + container_idx: 3, + key: 255, + value: Null, + }, + Map { + site: 1, + container_idx: 0, + key: 0, + value: Null, + }, + List { + site: 0, + container_idx: 3, + key: 0, + value: Container(C::Map), + }, + Map { + site: 0, + container_idx: 1, + key: 191, + value: Container(C::Text), + }, + ], + ) + } + + #[test] + fn fuzz_11_checkout() { + test_multi_sites( + 5, + &mut [ + Map { + site: 4, + container_idx: 0, + key: 8, + value: Null, + }, + Map { + site: 1, + container_idx: 0, + key: 8, + value: Container(C::Text), + }, + SyncAll, + Text { + site: 2, + container_idx: 1, + pos: 0, + value: 1918, + is_del: false, + }, + SyncAll, + Text { + site: 1, + container_idx: 1, + pos: 5, + value: 1, + is_del: true, + }, + Sync { from: 1, to: 2 }, + Text { + site: 2, + container_idx: 1, + pos: 1, + value: 5, + is_del: true, + }, + ], + ) + } + #[test] fn to_minify() { minify_error(5, vec![], test_multi_sites, normalize) diff --git a/crates/loro-internal/src/state.rs b/crates/loro-internal/src/state.rs index eaaf22f5..d0922ad4 100644 --- a/crates/loro-internal/src/state.rs +++ b/crates/loro-internal/src/state.rs @@ -253,6 +253,10 @@ impl DocState { DiffVariant::External(Diff::List(Delta::default())), ); + if diff.reset { + *state = create_state(diff.idx); + } + if is_recording { let external_diff = state .apply_diff_and_convert(internal_diff.into_internal().unwrap(), &self.arena); @@ -352,11 +356,14 @@ impl DocState { } if self.is_recording() { + debug_log::debug_log!("TODIFF"); let diff = self .states .iter_mut() .map(|(&idx, state)| InternalContainerDiff { idx, + reset: true, + is_container_deleted: false, diff: state.to_diff().into(), }) .collect(); @@ -590,7 +597,7 @@ impl DocState { // Because we need to calculate path based on [DocState], so we cannot extract // the event recorder to a separate module. - fn diffs_to_event(&self, diffs: Vec>, from: Frontiers) -> DocDiff { + fn diffs_to_event(&mut self, diffs: Vec>, from: Frontiers) -> DocDiff { if diffs.is_empty() { panic!("diffs is empty"); } @@ -602,6 +609,11 @@ impl DocState { for diff in diffs { #[allow(clippy::unnecessary_to_owned)] for container_diff in diff.diff.into_owned() { + if container_diff.is_container_deleted { + // omit event form deleted container + continue; + } + let Some((last_container_diff, _)) = containers.get_mut(&container_diff.idx) else { if let Some(path) = self.get_path(container_diff.idx) { containers.insert(container_diff.idx, (container_diff.diff, path)); @@ -609,8 +621,9 @@ impl DocState { // if we cannot find the path to the container, the container must be overwritten afterwards. // So we can ignore the diff from it. debug_log::debug_log!( - "⚠️ WARNING: ignore because cannot find path {:#?}", - &container_diff + "⚠️ WARNING: ignore because cannot find path {:#?} deep_value {:#?}", + &container_diff, + self.get_deep_value_with_id() ); } continue; @@ -659,7 +672,10 @@ impl DocState { debug_log::debug_dbg!(&id); if let Some(parent_idx) = self.arena.get_parent(idx) { let parent_state = self.states.get(&parent_idx).unwrap(); - let prop = parent_state.get_child_index(&id)?; + let Some(prop) = parent_state.get_child_index(&id) else { + debug_log::group_end!(); + return None; + }; ans.push((id, prop)); idx = parent_idx; } else { diff --git a/crates/loro-internal/src/state/list_state.rs b/crates/loro-internal/src/state/list_state.rs index fa1c9676..88f860c6 100644 --- a/crates/loro-internal/src/state/list_state.rs +++ b/crates/loro-internal/src/state/list_state.rs @@ -150,7 +150,7 @@ impl ListState { } pub fn get_child_container_index(&self, id: &ContainerID) -> Option { - let leaf = *self.child_container_to_leaf.get(id).unwrap(); + let leaf = *self.child_container_to_leaf.get(id)?; self.list.get_elem(leaf)?; let mut index = 0; self.list diff --git a/crates/loro-internal/src/state/richtext_state.rs b/crates/loro-internal/src/state/richtext_state.rs index 47ec351e..a620d506 100644 --- a/crates/loro-internal/src/state/richtext_state.rs +++ b/crates/loro-internal/src/state/richtext_state.rs @@ -94,8 +94,6 @@ impl ContainerState for RichtextState { unreachable!() }; - debug_log::group!("apply richtext diff and convert"); - debug_log::debug_dbg!(&richtext); let mut ans: Delta = Delta::new(); let mut entity_index = 0; let mut event_index = 0; @@ -240,8 +238,6 @@ impl ContainerState for RichtextState { } } - debug_log::debug_dbg!(&ans); - debug_log::group_end!(); Diff::Text(ans) } diff --git a/crates/loro-internal/src/txn.rs b/crates/loro-internal/src/txn.rs index 030d9b95..33d3b2eb 100644 --- a/crates/loro-internal/src/txn.rs +++ b/crates/loro-internal/src/txn.rs @@ -10,14 +10,9 @@ use fxhash::FxHashMap; use loro_common::{ContainerType, LoroResult}; use rle::{HasLength, RleVec}; - use crate::{ change::{get_sys_timestamp, Change, Lamport, Timestamp}, - container::{ - idx::ContainerIdx, - richtext::{Style}, - IntoContainerId, - }, + container::{idx::ContainerIdx, richtext::Style, IntoContainerId}, delta::{Delta, MapValue, TreeDelta, TreeDiff}, event::Diff, id::{Counter, PeerID, ID}, @@ -30,7 +25,7 @@ use crate::{ use super::{ arena::SharedArena, event::{InternalContainerDiff, InternalDocDiff}, - handler::{ListHandler, MapHandler,TreeHandler,TextHandler }, + handler::{ListHandler, MapHandler, TextHandler, TreeHandler}, oplog::OpLog, state::{DocState, State}, }; @@ -215,6 +210,8 @@ impl Transaction { arr.into_iter() .map(|x| InternalContainerDiff { idx: x.idx, + reset: false, + is_container_deleted: false, diff: x.diff.into(), }) .collect(), @@ -354,44 +351,44 @@ fn change_to_diff( 'outer: { let diff: Diff = match hint { - EventHint::Mark { start, end, style } => { - Diff::Text(Delta::new().retain(start).retain_with_meta( - end - start, - crate::delta::StyleMeta { vec: vec![style] }, - )) - } - EventHint::InsertText { pos, styles } => { - let slice = op.content.as_list().unwrap().as_insert_text().unwrap().0; - Diff::Text(Delta::new().retain(pos).insert_with_meta( - slice.clone(), - crate::delta::StyleMeta { vec: styles }, - )) - } - EventHint::DeleteText { pos, len } => { - Diff::Text(Delta::new().retain(pos).delete(len)) - } - EventHint::InsertList { pos, value } => { - Diff::List(Delta::new().retain(pos).insert(vec![value])) - } - EventHint::DeleteList { pos, len } => { - Diff::List(Delta::new().retain(pos).delete(len)) - } - EventHint::Map { key, value } => { - Diff::NewMap(crate::delta::MapDelta::new().with_entry( - key, - MapValue { - counter: op.counter, - value, - lamport: (lamport, peer), - }, - )) - } - EventHint::Tree(tree_diff) => Diff::Tree(TreeDelta::default().push(tree_diff)), - EventHint::None => { - // do nothing - break 'outer; - } - }; + EventHint::Mark { start, end, style } => { + Diff::Text(Delta::new().retain(start).retain_with_meta( + end - start, + crate::delta::StyleMeta { vec: vec![style] }, + )) + } + EventHint::InsertText { pos, styles } => { + let slice = op.content.as_list().unwrap().as_insert_text().unwrap().0; + Diff::Text(Delta::new().retain(pos).insert_with_meta( + slice.clone(), + crate::delta::StyleMeta { vec: styles }, + )) + } + EventHint::DeleteText { pos, len } => { + Diff::Text(Delta::new().retain(pos).delete(len)) + } + EventHint::InsertList { pos, value } => { + Diff::List(Delta::new().retain(pos).insert(vec![value])) + } + EventHint::DeleteList { pos, len } => { + Diff::List(Delta::new().retain(pos).delete(len)) + } + EventHint::Map { key, value } => { + Diff::NewMap(crate::delta::MapDelta::new().with_entry( + key, + MapValue { + counter: op.counter, + value, + lamport: (lamport, peer), + }, + )) + } + EventHint::Tree(tree_diff) => Diff::Tree(TreeDelta::default().push(tree_diff)), + EventHint::None => { + // do nothing + break 'outer; + } + }; ans.push(TxnContainerDiff { idx: op.container, diff --git a/crates/loro-internal/tests/test.rs b/crates/loro-internal/tests/test.rs index 1093d2d2..9f846144 100644 --- a/crates/loro-internal/tests/test.rs +++ b/crates/loro-internal/tests/test.rs @@ -1,7 +1,58 @@ -use std::sync::Arc; +use std::sync::{Arc, Mutex}; -use loro_common::ID; -use loro_internal::{version::Frontiers, LoroDoc, ToJson}; +use loro_common::{ContainerType, LoroValue, ID}; +use loro_internal::{version::Frontiers, ApplyDiff, LoroDoc, ToJson}; +use serde_json::json; + +#[test] +fn test_checkout() { + let mut doc_0 = LoroDoc::new(); + doc_0.set_peer_id(0); + let doc_1 = LoroDoc::new(); + doc_1.set_peer_id(1); + + let value: Arc> = Arc::new(Mutex::new(LoroValue::Map(Default::default()))); + let root_value = value.clone(); + doc_0.subscribe_deep(Arc::new(move |event| { + let mut root_value = root_value.lock().unwrap(); + root_value.apply( + &event.container.path.iter().map(|x| x.1.clone()).collect(), + &[event.container.diff.clone()], + ); + })); + + let map = doc_0.get_map("map"); + doc_0 + .with_txn(|txn| { + let handler = map.insert_container(txn, "text", ContainerType::Text)?; + let text = handler.into_text().unwrap(); + text.insert(txn, 0, "123") + }) + .unwrap(); + + let map = doc_1.get_map("map"); + doc_1 + .with_txn(|txn| map.insert(txn, "text", LoroValue::Double(1.0))) + .unwrap(); + + doc_0 + .import(&doc_1.export_from(&Default::default())) + .unwrap(); + + doc_0 + .checkout(&Frontiers::from(vec![ID::new(0, 2)])) + .unwrap(); + + assert_eq!(&doc_0.get_deep_value(), &*value.lock().unwrap()); + assert_eq!( + value.lock().unwrap().to_json_value(), + json!({ + "map": { + "text": "12" + } + }) + ); +} #[test] fn import() {