From 8dcb6191470acedf3cd75682a4fd7dfac9326d41 Mon Sep 17 00:00:00 2001 From: Zixuan Chen Date: Tue, 17 Sep 2024 12:09:20 +0800 Subject: [PATCH] fix: raise error if perform action on a deleted container (#465) - Add ContainerDeleted error variant to LoroError - Implement is_deleted() method for container handlers - Check for deleted containers before applying operations - Add dead_containers_cache to DocState to track deleted containers - Update apply_diff to optionally clear dead container cache - Add tests for handling operations on deleted containers - Implement checkout to reset container deleted cache - Minor code cleanup and typo fixes --- crates/fuzz/src/container/counter.rs | 2 +- crates/fuzz/src/container/list.rs | 7 +- crates/fuzz/src/container/map.rs | 8 +- crates/fuzz/src/container/mod.rs | 10 + crates/fuzz/src/container/movable_list.rs | 14 +- crates/fuzz/src/container/text.rs | 10 +- crates/fuzz/src/container/tree.rs | 15 +- crates/fuzz/tests/test.rs | 224 ++++++++++++++++++ crates/loro-common/src/error.rs | 6 +- crates/loro-internal/src/handler.rs | 54 ++++- crates/loro-internal/src/handler/tree.rs | 7 + crates/loro-internal/src/loro.rs | 45 ++-- crates/loro-internal/src/state.rs | 31 ++- .../src/state/dead_containers_cache.rs | 49 ++++ crates/loro-internal/src/txn.rs | 5 + crates/loro/src/lib.rs | 5 + crates/loro/tests/loro_rust_test.rs | 28 +++ 17 files changed, 462 insertions(+), 58 deletions(-) create mode 100644 crates/loro-internal/src/state/dead_containers_cache.rs diff --git a/crates/fuzz/src/container/counter.rs b/crates/fuzz/src/container/counter.rs index d2c0c0aa..804aea50 100644 --- a/crates/fuzz/src/container/counter.rs +++ b/crates/fuzz/src/container/counter.rs @@ -113,7 +113,7 @@ impl Actionable for CounterAction { fn apply(&self, actor: &mut ActionExecutor, container: usize) -> Option { let actor = actor.as_counter_actor_mut().unwrap(); let counter = actor.containers.get(container).unwrap(); - counter.increment(self.0 as f64).unwrap(); + super::unwrap(counter.increment(self.0 as f64)); None } diff --git a/crates/fuzz/src/container/list.rs b/crates/fuzz/src/container/list.rs index 0698357d..4b895d50 100644 --- a/crates/fuzz/src/container/list.rs +++ b/crates/fuzz/src/container/list.rs @@ -112,11 +112,10 @@ impl Actionable for ListAction { let pos = *pos as usize; match value { FuzzValue::Container(c) => { - let container = list.insert_container(pos, Container::new(*c)).unwrap(); - Some(container) + super::unwrap(list.insert_container(pos, Container::new(*c))) } FuzzValue::I32(v) => { - list.insert(pos, *v).unwrap(); + super::unwrap(list.insert(pos, *v)); None } } @@ -124,7 +123,7 @@ impl Actionable for ListAction { ListAction::Delete { pos, len } => { let pos = *pos as usize; let len = *len as usize; - list.delete(pos, len).unwrap(); + super::unwrap(list.delete(pos, len)); None } } diff --git a/crates/fuzz/src/container/map.rs b/crates/fuzz/src/container/map.rs index 8be43e85..9bc81224 100644 --- a/crates/fuzz/src/container/map.rs +++ b/crates/fuzz/src/container/map.rs @@ -135,22 +135,22 @@ impl Actionable for MapAction { fn apply(&self, actor: &mut ActionExecutor, container: usize) -> Option { let actor = actor.as_map_actor_mut().unwrap(); let handler = actor.get_create_container_mut(container); + use super::unwrap; match self { MapAction::Insert { key, value, .. } => { let key = &key.to_string(); match value { FuzzValue::I32(v) => { - handler.insert(key, LoroValue::from(*v)).unwrap(); + unwrap(handler.insert(key, LoroValue::from(*v))); None } FuzzValue::Container(c) => { - let container = handler.insert_container(key, Container::new(*c)).unwrap(); - Some(container) + unwrap(handler.insert_container(key, Container::new(*c))) } } } MapAction::Delete { key, .. } => { - handler.delete(&key.to_string()).unwrap(); + unwrap(handler.delete(&key.to_string())); None } } diff --git a/crates/fuzz/src/container/mod.rs b/crates/fuzz/src/container/mod.rs index 402e8bcc..bd2ceb49 100644 --- a/crates/fuzz/src/container/mod.rs +++ b/crates/fuzz/src/container/mod.rs @@ -6,7 +6,17 @@ mod text; mod tree; pub use counter::*; pub use list::*; +use loro::{LoroError, LoroResult}; pub use map::*; pub use movable_list::*; pub use text::*; pub use tree::*; + +/// ignore_container_delete_error +fn unwrap(r: LoroResult) -> Option { + match r { + Ok(v) => Some(v), + Err(LoroError::ContainerDeleted { .. }) => None, + Err(e) => panic!("Error: {}", e), + } +} diff --git a/crates/fuzz/src/container/movable_list.rs b/crates/fuzz/src/container/movable_list.rs index 15bf8d5c..0ff74fbe 100644 --- a/crates/fuzz/src/container/movable_list.rs +++ b/crates/fuzz/src/container/movable_list.rs @@ -135,11 +135,10 @@ impl Actionable for MovableListAction { let pos = *pos as usize; match value { FuzzValue::Container(c) => { - let container = list.insert_container(pos, Container::new(*c)).unwrap(); - Some(container) + super::unwrap(list.insert_container(pos, Container::new(*c))) } FuzzValue::I32(v) => { - list.insert(pos, *v).unwrap(); + super::unwrap(list.insert(pos, *v)); None } } @@ -147,24 +146,23 @@ impl Actionable for MovableListAction { MovableListAction::Delete { pos, len } => { let pos = *pos as usize; let len = *len as usize; - list.delete(pos, len).unwrap(); + super::unwrap(list.delete(pos, len)); None } MovableListAction::Move { from, to } => { let from = *from as usize; let to = *to as usize; - list.mov(from, to).unwrap(); + super::unwrap(list.mov(from, to)); None } MovableListAction::Set { pos, value } => { let pos = *pos as usize; match value { FuzzValue::Container(c) => { - let container = list.set_container(pos, Container::new(*c)).unwrap(); - Some(container) + super::unwrap(list.set_container(pos, Container::new(*c))) } FuzzValue::I32(v) => { - list.set(pos, *v).unwrap(); + super::unwrap(list.set(pos, *v)); None } } diff --git a/crates/fuzz/src/container/text.rs b/crates/fuzz/src/container/text.rs index 1345be54..250b702e 100644 --- a/crates/fuzz/src/container/text.rs +++ b/crates/fuzz/src/container/text.rs @@ -121,14 +121,16 @@ impl Actionable for TextAction { let actor = actor.as_text_actor_mut().unwrap(); let text = actor.containers.get(container).unwrap(); let TextAction { pos, len, action } = self; + use super::unwrap; match action { - TextActionInner::Insert => text.insert(*pos, &format!("[{}]", len)).unwrap(), + TextActionInner::Insert => { + unwrap(text.insert(*pos, &format!("[{}]", len))); + } TextActionInner::Delete => { - text.delete(*pos, *len).unwrap(); + unwrap(text.delete(*pos, *len)); } TextActionInner::Mark(i) => { - text.mark(*pos..*pos + *len, STYLES_NAME[*i], *pos as i32) - .unwrap(); + unwrap(text.mark(*pos..*pos + *len, STYLES_NAME[*i], *pos as i32)); } } None diff --git a/crates/fuzz/src/container/tree.rs b/crates/fuzz/src/container/tree.rs index 8520efbd..1caaea39 100644 --- a/crates/fuzz/src/container/tree.rs +++ b/crates/fuzz/src/container/tree.rs @@ -241,11 +241,11 @@ impl Actionable for TreeAction { tree.enable_fractional_index(0); match action { TreeActionInner::Create { index } => { - tree.create_at(None, *index).unwrap(); + super::unwrap(tree.create_at(None, *index)); None } TreeActionInner::Delete => { - tree.delete(target).unwrap(); + super::unwrap(tree.delete(target)); None } TreeActionInner::Move { parent, index } => { @@ -268,7 +268,7 @@ impl Actionable for TreeAction { peer: before.0, counter: before.1, }; - tree.mov_before(target, before).unwrap(); + super::unwrap(tree.mov_before(target, before)); None } TreeActionInner::MoveAfter { target, after } => { @@ -280,19 +280,18 @@ impl Actionable for TreeAction { peer: after.0, counter: after.1, }; - tree.mov_after(target, after).unwrap(); + super::unwrap(tree.mov_after(target, after)); None } TreeActionInner::Meta { meta: (k, v) } => { - let meta = tree.get_meta(target).unwrap(); + let meta = super::unwrap(tree.get_meta(target))?; match v { FuzzValue::I32(i) => { - meta.insert(k, LoroValue::from(*i)).unwrap(); + super::unwrap(meta.insert(k, LoroValue::from(*i))); None } FuzzValue::Container(c) => { - let container = meta.insert_container(k, Container::new(*c)).unwrap(); - Some(container) + super::unwrap(meta.insert_container(k, Container::new(*c))) } } } diff --git a/crates/fuzz/tests/test.rs b/crates/fuzz/tests/test.rs index b61baa42..56181911 100644 --- a/crates/fuzz/tests/test.rs +++ b/crates/fuzz/tests/test.rs @@ -10560,6 +10560,230 @@ fn gc_fuzz_19() { ], ) } + +#[test] +fn gc_fuzz_20() { + test_multi_sites_with_gc( + 5, + vec![FuzzTarget::All], + &mut [ + SyncAll, + SyncAll, + Handle { + site: 27, + target: 229, + container: 228, + action: Generic(GenericAction { + value: Container(Text), + bool: true, + key: 4288224017, + pos: 2017611710352523263, + length: 2889933389121133339, + prop: 1953184666628070171, + }), + }, + Handle { + site: 27, + target: 27, + container: 27, + action: Generic(GenericAction { + value: I32(454762267), + bool: true, + key: 2300255003, + pos: 1953184666628069915, + length: 1953198960279231259, + prop: 16637674123620458267, + }), + }, + Handle { + site: 27, + target: 27, + container: 27, + action: Generic(GenericAction { + value: I32(455613211), + bool: true, + key: 454761243, + pos: 1953279224628058907, + length: 1953184666628070171, + prop: 13093570503287832689, + }), + }, + Sync { from: 181, to: 181 }, + Undo { + site: 113, + op_len: 3048545137, + }, + Sync { from: 181, to: 181 }, + Handle { + site: 27, + target: 27, + container: 27, + action: Generic(GenericAction { + value: I32(454761243), + bool: true, + key: 469048091, + pos: 1953305612907387675, + length: 1953184666611321115, + prop: 2889933389121133339, + }), + }, + Handle { + site: 27, + target: 119, + container: 228, + action: Generic(GenericAction { + value: I32(454761242), + bool: true, + key: 454761243, + pos: 1953184666846173979, + length: 8150137753889872667, + prop: 1953184666628070171, + }), + }, + Handle { + site: 2, + target: 0, + container: 181, + action: Generic(GenericAction { + value: Container(Tree), + bool: true, + key: 3048584629, + pos: 13093571281108186549, + length: 13093571283691877813, + prop: 8174439530702681525, + }), + }, + Sync { from: 181, to: 181 }, + Sync { from: 181, to: 181 }, + Handle { + site: 181, + target: 181, + container: 181, + action: Generic(GenericAction { + value: Container(Tree), + bool: true, + key: 1903277493, + pos: 31836475601132401, + length: 13093571283679969793, + prop: 13093571283691877813, + }), + }, + Handle { + site: 181, + target: 181, + container: 181, + action: Generic(GenericAction { + value: Container(Tree), + bool: true, + key: 1903277493, + pos: 13093570621121589617, + length: 13093571283691877813, + prop: 13050224137278436789, + }), + }, + Handle { + site: 181, + target: 181, + container: 181, + action: Generic(GenericAction { + value: Container(Tree), + bool: true, + key: 3941264106, + pos: 1982722105925435391, + length: 8150137753889872667, + prop: 1953184666628070171, + }), + }, + Sync { from: 181, to: 181 }, + Sync { from: 181, to: 181 }, + Sync { from: 181, to: 27 }, + Sync { from: 181, to: 181 }, + Sync { from: 181, to: 181 }, + Undo { + site: 255, + op_len: 4294967295, + }, + Handle { + site: 181, + target: 181, + container: 181, + action: Generic(GenericAction { + value: Container(Tree), + bool: true, + key: 2307503541, + pos: 15553137160186484565, + length: 13093571283691831297, + prop: 1953185037443773877, + }), + }, + Handle { + site: 2, + target: 0, + container: 181, + action: Generic(GenericAction { + value: Container(Tree), + bool: true, + key: 3048584629, + pos: 13093571281108186549, + length: 13055572161835939253, + prop: 8174439530707137973, + }), + }, + Sync { from: 181, to: 181 }, + Sync { from: 181, to: 181 }, + Sync { from: 27, to: 181 }, + Handle { + site: 181, + target: 181, + container: 181, + action: Generic(GenericAction { + value: Container(Tree), + bool: true, + key: 3941264106, + pos: 1982722105925435391, + length: 8150137753889872667, + prop: 1953184666628070171, + }), + }, + Sync { from: 181, to: 181 }, + Sync { from: 181, to: 181 }, + Sync { from: 181, to: 27 }, + Sync { from: 181, to: 181 }, + Sync { from: 181, to: 181 }, + Undo { + site: 255, + op_len: 4294967295, + }, + Handle { + site: 181, + target: 181, + container: 181, + action: Generic(GenericAction { + value: Container(Tree), + bool: true, + key: 2307503541, + pos: 15553137160186484565, + length: 8215928015238070273, + prop: 1953184666635301490, + }), + }, + Handle { + site: 35, + target: 35, + container: 35, + action: Generic(GenericAction { + value: I32(589505315), + bool: true, + key: 589505315, + pos: 590711300899, + length: 0, + prop: 0, + }), + }, + ], + ) +} + #[test] fn minify() { minify_error( diff --git a/crates/loro-common/src/error.rs b/crates/loro-common/src/error.rs index 5f716325..1cb34cff 100644 --- a/crates/loro-common/src/error.rs +++ b/crates/loro-common/src/error.rs @@ -1,7 +1,7 @@ use serde_columnar::ColumnarError; use thiserror::Error; -use crate::{InternalString, PeerID, TreeID, ID}; +use crate::{ContainerID, InternalString, PeerID, TreeID, ID}; pub type LoroResult = Result; @@ -80,6 +80,10 @@ pub enum LoroError { ImportUpdatesThatDependsOnOutdatedVersion, #[error("You cannot switch a document to a version before the trimmed version.")] SwitchToTrimmedVersion, + #[error( + "The container {container} is deleted. You cannot apply the op on a deleted container." + )] + ContainerDeleted { container: Box }, } #[derive(Error, Debug, PartialEq)] diff --git a/crates/loro-internal/src/handler.rs b/crates/loro-internal/src/handler.rs index deb1c00c..a16c40ec 100644 --- a/crates/loro-internal/src/handler.rs +++ b/crates/loro-internal/src/handler.rs @@ -293,6 +293,13 @@ impl BasicHandler { pub fn parent(&self) -> Option { self.get_parent() } + + fn is_deleted(&self) -> bool { + match self.state.upgrade() { + None => false, + Some(state) => state.lock().unwrap().is_deleted(self.container_idx), + } + } } /// Flatten attributes that allow overlap @@ -579,7 +586,7 @@ impl HandlerTrait for MapHandler { impl std::fmt::Debug for MapHandler { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.inner { - MaybeDetached::Detached(_) => write!(f, "MapHandler Dettached"), + MaybeDetached::Detached(_) => write!(f, "MapHandler Detached"), MaybeDetached::Attached(a) => write!(f, "MapHandler {}", a.id), } } @@ -705,7 +712,7 @@ impl std::fmt::Debug for MovableListHandler { impl std::fmt::Debug for ListHandler { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.inner { - MaybeDetached::Detached(_) => write!(f, "ListHandler Dettached"), + MaybeDetached::Detached(_) => write!(f, "ListHandler Detached"), MaybeDetached::Attached(a) => write!(f, "ListHandler {}", a.id), } } @@ -819,6 +826,12 @@ impl Debug for UnknownHandler { } } +impl UnknownHandler { + pub fn is_deleted(&self) -> bool { + self.inner.is_deleted() + } +} + impl HandlerTrait for UnknownHandler { fn is_attached(&self) -> bool { true @@ -2265,6 +2278,13 @@ impl TextHandler { }) .unwrap() } + + pub fn is_deleted(&self) -> bool { + match &self.inner { + MaybeDetached::Detached(_) => false, + MaybeDetached::Attached(a) => a.is_deleted(), + } + } } fn event_len(s: &str) -> usize { @@ -2678,6 +2698,13 @@ impl ListHandler { Ok(()) } + + pub fn is_deleted(&self) -> bool { + match &self.inner { + MaybeDetached::Detached(_) => false, + MaybeDetached::Attached(a) => a.is_deleted(), + } + } } impl MovableListHandler { @@ -3310,6 +3337,13 @@ impl MovableListHandler { } } } + + pub fn is_deleted(&self) -> bool { + match &self.inner { + MaybeDetached::Detached(_) => false, + MaybeDetached::Attached(a) => a.is_deleted(), + } + } } impl MapHandler { @@ -3612,6 +3646,13 @@ impl MapHandler { pub fn is_empty(&self) -> bool { self.len() == 0 } + + pub fn is_deleted(&self) -> bool { + match &self.inner { + MaybeDetached::Detached(_) => false, + MaybeDetached::Attached(a) => a.is_deleted(), + } + } } #[inline(always)] @@ -3683,12 +3724,19 @@ pub mod counter { &inner.state, ) } + + pub fn is_deleted(&self) -> bool { + match &self.inner { + MaybeDetached::Detached(_) => false, + MaybeDetached::Attached(a) => a.is_deleted(), + } + } } impl std::fmt::Debug for CounterHandler { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match &self.inner { - MaybeDetached::Detached(_) => write!(f, "CounterHandler Dettached"), + MaybeDetached::Detached(_) => write!(f, "CounterHandler Detached"), MaybeDetached::Attached(a) => write!(f, "CounterHandler {}", a.id), } } diff --git a/crates/loro-internal/src/handler/tree.rs b/crates/loro-internal/src/handler/tree.rs index 6a6a5076..52858961 100644 --- a/crates/loro-internal/src/handler/tree.rs +++ b/crates/loro-internal/src/handler/tree.rs @@ -914,4 +914,11 @@ impl TreeHandler { }), } } + + pub fn is_deleted(&self) -> bool { + match &self.inner { + MaybeDetached::Detached(_) => false, + MaybeDetached::Attached(a) => a.is_deleted(), + } + } } diff --git a/crates/loro-internal/src/loro.rs b/crates/loro-internal/src/loro.rs index 4bb8360d..745dfd7e 100644 --- a/crates/loro-internal/src/loro.rs +++ b/crates/loro-internal/src/loro.rs @@ -570,12 +570,15 @@ impl LoroDoc { None, ); let mut state = self.state.lock().unwrap(); - state.apply_diff(InternalDocDiff { - origin, - diff: (diff).into(), - by: EventTriggerKind::Import, - new_version: Cow::Owned(oplog.frontiers().clone()), - }); + state.apply_diff( + InternalDocDiff { + origin, + diff: (diff).into(), + by: EventTriggerKind::Import, + new_version: Cow::Owned(oplog.frontiers().clone()), + }, + false, + ); } } else { tracing::info!("Detached"); @@ -607,12 +610,15 @@ impl LoroDoc { None, ); let mut state = self.state.lock().unwrap(); - state.apply_diff(InternalDocDiff { - origin: "".into(), - diff: (diff).into(), - by: EventTriggerKind::Import, - new_version: Cow::Owned(oplog.frontiers().clone()), - }); + state.apply_diff( + InternalDocDiff { + origin: "".into(), + diff: (diff).into(), + by: EventTriggerKind::Import, + new_version: Cow::Owned(oplog.frontiers().clone()), + }, + false, + ); } self.renew_txn_if_auto_commit(); ans @@ -1182,12 +1188,15 @@ impl LoroDoc { Some(&frontiers), None, ); - state.apply_diff(InternalDocDiff { - origin: "checkout".into(), - diff: Cow::Owned(diff), - by: EventTriggerKind::Checkout, - new_version: Cow::Owned(frontiers.clone()), - }); + state.apply_diff( + InternalDocDiff { + origin: "checkout".into(), + diff: Cow::Owned(diff), + by: EventTriggerKind::Checkout, + new_version: Cow::Owned(frontiers.clone()), + }, + true, + ); Ok(()) } diff --git a/crates/loro-internal/src/state.rs b/crates/loro-internal/src/state.rs index 76d488ae..a46d6de9 100644 --- a/crates/loro-internal/src/state.rs +++ b/crates/loro-internal/src/state.rs @@ -9,6 +9,7 @@ use std::{ }; use container_store::ContainerStore; +use dead_containers_cache::DeadContainersCache; use enum_as_inner::EnumAsInner; use enum_dispatch::enum_dispatch; use fxhash::{FxHashMap, FxHashSet}; @@ -38,6 +39,7 @@ pub(crate) mod analyzer; pub(crate) mod container_store; #[cfg(feature = "counter")] mod counter_state; +mod dead_containers_cache; mod list_state; mod map_state; mod movable_list_state; @@ -77,6 +79,8 @@ pub struct DocState { // diff related stuff event_recorder: EventRecorder, + + dead_containers_cache: DeadContainersCache, } impl std::fmt::Debug for DocState { @@ -354,6 +358,7 @@ impl DocState { in_txn: false, changed_idx_in_txn: FxHashSet::default(), event_recorder: Default::default(), + dead_containers_cache: Default::default(), }) }) } @@ -377,6 +382,7 @@ impl DocState { in_txn: false, changed_idx_in_txn: FxHashSet::default(), event_recorder: Default::default(), + dead_containers_cache: Default::default(), }) }) } @@ -488,11 +494,19 @@ impl DocState { /// It's expected that diff only contains [`InternalDiff`] /// #[instrument(skip_all)] - pub(crate) fn apply_diff(&mut self, mut diff: InternalDocDiff<'static>) { + pub(crate) fn apply_diff( + &mut self, + mut diff: InternalDocDiff<'static>, + need_clear_dead_container_cache: bool, + ) { if self.in_txn { panic!("apply_diff should not be called in a transaction"); } + if need_clear_dead_container_cache { + self.dead_containers_cache.clear(); + } + let is_recording = self.is_recording(); self.pre_txn(diff.origin.clone(), diff.by); let Cow::Owned(mut diffs) = std::mem::take(&mut diff.diff) else { @@ -810,12 +824,15 @@ impl DocState { Some(&frontiers), Some(&|idx| !idx.is_unknown() && unknown_containers.contains(&idx)), ); - self.apply_diff(InternalDocDiff { - origin: Default::default(), - by: EventTriggerKind::Import, - diff: unknown_diffs.into(), - new_version: Cow::Owned(frontiers.clone()), - }) + self.apply_diff( + InternalDocDiff { + origin: Default::default(), + by: EventTriggerKind::Import, + diff: unknown_diffs.into(), + new_version: Cow::Owned(frontiers.clone()), + }, + false, + ) } if self.is_recording() { diff --git a/crates/loro-internal/src/state/dead_containers_cache.rs b/crates/loro-internal/src/state/dead_containers_cache.rs new file mode 100644 index 00000000..dc325947 --- /dev/null +++ b/crates/loro-internal/src/state/dead_containers_cache.rs @@ -0,0 +1,49 @@ +use super::{ContainerState, DocState}; +use crate::container::idx::ContainerIdx; +use fxhash::FxHashSet; + +#[derive(Default, Debug, Clone)] +pub(super) struct DeadContainersCache { + deleted_containers: FxHashSet, +} + +impl DeadContainersCache { + pub fn clear(&mut self) { + self.deleted_containers.clear(); + } +} + +impl DocState { + pub(crate) fn is_deleted(&mut self, idx: ContainerIdx) -> bool { + if self.dead_containers_cache.deleted_containers.contains(&idx) { + return true; + } + + let mut visited = vec![idx]; + let mut idx = idx; + let is_deleted = loop { + let id = self.arena.idx_to_id(idx).unwrap(); + if let Some(parent_idx) = self.arena.get_parent(idx) { + let Some(parent_state) = self.store.get_container_mut(parent_idx) else { + break true; + }; + if !parent_state.contains_child(&id) { + break true; + } + + idx = parent_idx; + visited.push(idx); + } else { + break !id.is_root(); + } + }; + + if is_deleted { + for idx in visited { + self.dead_containers_cache.deleted_containers.insert(idx); + } + } + + is_deleted + } +} diff --git a/crates/loro-internal/src/txn.rs b/crates/loro-internal/src/txn.rs index ed64b766..59ce04b6 100644 --- a/crates/loro-internal/src/txn.rs +++ b/crates/loro-internal/src/txn.rs @@ -416,6 +416,11 @@ impl Transaction { }; let mut state = self.state.lock().unwrap(); + if state.is_deleted(container) { + return Err(LoroError::ContainerDeleted { + container: Box::new(state.arena.idx_to_id(container).unwrap()), + }); + } let op = self.arena.convert_raw_op(&raw_op); state.apply_local_op(&raw_op, &op)?; drop(state); diff --git a/crates/loro/src/lib.rs b/crates/loro/src/lib.rs index a30bd05e..0fbd2c8f 100644 --- a/crates/loro/src/lib.rs +++ b/crates/loro/src/lib.rs @@ -1382,6 +1382,11 @@ impl LoroText { pub fn get_cursor(&self, pos: usize, side: Side) -> Option { self.handler.get_cursor(pos, side) } + + /// Whether the text container is deleted. + pub fn is_deleted(&self) -> bool { + self.handler.is_deleted() + } } impl Default for LoroText { diff --git a/crates/loro/tests/loro_rust_test.rs b/crates/loro/tests/loro_rust_test.rs index a1117d3b..ef3321e0 100644 --- a/crates/loro/tests/loro_rust_test.rs +++ b/crates/loro/tests/loro_rust_test.rs @@ -1634,3 +1634,31 @@ fn test_get_shallow_value() { assert!(v.contains_key("text")); assert!(v.contains_key("movable_list")); } + +#[test] +fn perform_action_on_deleted_container_should_return_error() { + let doc = LoroDoc::new(); + let list = doc.get_movable_list("list"); + let text = list.push_container(LoroText::new()).unwrap(); + list.set(0, 1).unwrap(); + let result = text.insert(0, "Hello"); + match result { + Ok(_) => panic!("Expected error, but operation succeeded"), + Err(LoroError::ContainerDeleted { .. }) => {} + _ => panic!("Expected ContainerDeleted error, but got something else"), + } + assert!(text.is_deleted()); +} + +#[test] +fn checkout_should_reset_container_deleted_cache() { + let doc = LoroDoc::new(); + let list = doc.get_movable_list("list"); + let text = list.push_container(LoroText::new()).unwrap(); + doc.commit(); + let f = doc.state_frontiers(); + list.set(0, 1).unwrap(); + assert!(text.is_deleted()); + doc.checkout(&f).unwrap(); + assert!(!text.is_deleted()); +}