mirrors/loro
1
0
Fork 0
mirror of https://github.com/loro-dev/loro.git synced 2025-02-05 20:17:13 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 2

fix: raise error if perform action on a deleted container (#465)

Browse source 
- 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
This commit is contained in:
Zixuan Chen 2024-09-17 12:09:20 +08:00 committed by GitHub
parent 2f38708a50
commit 8dcb619147
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
17 changed files with 462 additions and 58 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
crates/fuzz/src/container/counter.rs
View file

@ -113,7 +113,7 @@ impl Actionable for CounterAction {
fn apply(&self, actor: &mut ActionExecutor, container: usize) -> Option<Container> {
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
}

7
crates/fuzz/src/container/list.rs
View file

@ -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
}
}

8
crates/fuzz/src/container/map.rs
View file

@ -135,22 +135,22 @@ impl Actionable for MapAction {
fn apply(&self, actor: &mut ActionExecutor, container: usize) -> Option<Container> {
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
}
}

10
crates/fuzz/src/container/mod.rs
View file

@ -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<T>(r: LoroResult<T>) -> Option<T> {
match r {
Ok(v) => Some(v),
Err(LoroError::ContainerDeleted { .. }) => None,
Err(e) => panic!("Error: {}", e),
}
}

14
crates/fuzz/src/container/movable_list.rs
View file

@ -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
}
}

10
crates/fuzz/src/container/text.rs
View file

@ -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

15
crates/fuzz/src/container/tree.rs
View file

@ -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)))
}
}
}

224
crates/fuzz/tests/test.rs
View file

@ -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(

6
crates/loro-common/src/error.rs
View file

@ -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<T> = Result<T, LoroError>;
@ -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<ContainerID> },
}
#[derive(Error, Debug, PartialEq)]

54
crates/loro-internal/src/handler.rs
View file

@ -293,6 +293,13 @@ impl BasicHandler {
pub fn parent(&self) -> Option<Handler> {
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),
}
}

7
crates/loro-internal/src/handler/tree.rs
View file

@ -914,4 +914,11 @@ impl TreeHandler {
}),
}
}
pub fn is_deleted(&self) -> bool {
match &self.inner {
MaybeDetached::Detached(_) => false,
MaybeDetached::Attached(a) => a.is_deleted(),
}
}
}

45
crates/loro-internal/src/loro.rs
View file

@ -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(())
}

31
crates/loro-internal/src/state.rs
View file

@ -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() {

49
crates/loro-internal/src/state/dead_containers_cache.rs Normal file
View file

@ -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<ContainerIdx>,
}
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
}
}

5
crates/loro-internal/src/txn.rs
View file

@ -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);

5
crates/loro/src/lib.rs
View file

@ -1382,6 +1382,11 @@ impl LoroText {
pub fn get_cursor(&self, pos: usize, side: Side) -> Option<Cursor> {
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 {

28
crates/loro/tests/loro_rust_test.rs
View file

@ -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());
}