diff --git a/packages/common/y-octo/core/src/doc/document.rs b/packages/common/y-octo/core/src/doc/document.rs index baa6c0b0f..7e6c783b3 100644 --- a/packages/common/y-octo/core/src/doc/document.rs +++ b/packages/common/y-octo/core/src/doc/document.rs @@ -242,12 +242,29 @@ impl Doc { pub fn apply_update(&mut self, mut update: Update) -> JwstCodecResult { let mut store = self.store.write().unwrap(); let mut retry = false; + loop { + // clone every time to avoid ref count issue + let pending_types = update + .structs + .values() + .flatten() + .filter_map(|n| { + if let Node::Item(item_ref) = n + && let Some(item) = item_ref.get() + && let Content::Type(ty) = &item.content + { + Some((item.id, ty.clone())) + } else { + None + } + }) + .collect(); for (mut s, offset) in update.iter(store.get_state_vector()) { if let Node::Item(item) = &mut s { debug_assert!(item.is_owned()); let mut item = unsafe { item.get_mut_unchecked() }; - store.repair(&mut item, self.store.clone())?; + store.repair(&mut item, self.store.clone(), &pending_types)?; } store.integrate(s, offset, None)?; } @@ -528,6 +545,33 @@ mod tests { assert!(list.len() == 7); assert!(matches!(list[6], Value::Array(_))); } + + { + let binary_detached = { + let doc = Doc::new(); + let mut array = doc.get_or_create_array("abc").unwrap(); + array.insert(0, 42).unwrap(); + array.insert(1, -42).unwrap(); + array.insert(2, true).unwrap(); + array.insert(3, false).unwrap(); + array.insert(4, "hello").unwrap(); + array.insert(5, "world").unwrap(); + + let mut sub_array = doc.create_array().unwrap(); + sub_array.insert(0, 1).unwrap(); + array.insert(6, sub_array.clone()).unwrap(); + + doc.encode_update_v1().unwrap() + }; + + let detached_doc = Doc::try_from_binary_v1(binary_detached).unwrap(); + let detached_array = detached_doc.get_or_create_array("abc").unwrap(); + let detached_sub_array = match detached_array.get(6).unwrap() { + Value::Array(arr) => arr, + _ => panic!("expected array at index 6"), + }; + assert_eq!(detached_sub_array.get(0).unwrap(), Value::Any(1.0.into())); + } } #[test] @@ -639,12 +683,27 @@ mod tests { let mut text = doc.get_or_create_text("text").unwrap(); text.insert(0, "hello world").unwrap(); + let mut root = doc.get_or_create_map("root").unwrap(); + let mut child = doc.create_map().unwrap(); + child.insert("k".to_string(), "v").unwrap(); + root.insert("child".to_string(), child.clone()).unwrap(); + let update = doc.encode_update_v1().unwrap(); let doc = Doc::try_from_binary_v1(update).unwrap(); let text = doc.get_or_create_text("text").unwrap(); assert_eq!(&text.to_string(), "hello world"); + + let root = doc.get_or_create_map("root").unwrap(); + if let Some(Value::Map(child)) = root.get("child") { + assert!( + matches!(child.get("k"), Some(Value::Any(Any::String(s))) if s == "v"), + "expected nested map value to survive apply_update" + ); + } else { + panic!("expected nested map to survive apply_update"); + } }); } diff --git a/packages/common/y-octo/core/src/doc/store.rs b/packages/common/y-octo/core/src/doc/store.rs index 3c1b8d580..d3dc13ffd 100644 --- a/packages/common/y-octo/core/src/doc/store.rs +++ b/packages/common/y-octo/core/src/doc/store.rs @@ -11,6 +11,7 @@ use crate::{ }; pub type ChangedTypeRefs = HashMap>; +type PendingTypes = HashMap; unsafe impl Send for DocStore {} unsafe impl Sync for DocStore {} @@ -134,7 +135,7 @@ impl DocStore { return Err(JwstCodecError::StructClockInvalid { expect, actually }); } } else { - warn!("client {client_id} has no struct info"); + warn!("client {} has no struct info", client_id); } structs.push_back(item); } @@ -148,27 +149,28 @@ impl DocStore { /// binary search struct info on a sorted array pub fn get_node_index(items: &VecDeque, clock: Clock) -> Option { - let mut left = 0; - let mut right = items.len() - 1; - let middle = &items[right]; - let middle_clock = middle.clock(); - if middle_clock == clock { - return Some(right); + if items.is_empty() { + return None; } - let mut middle_index = (clock / (middle_clock + middle.len() - 1)) as usize * right; - while left <= right { + + let mut left = 0usize; + let mut right = items.len(); + + while left < right { + let middle_index = left + (right - left) / 2; let middle = &items[middle_index]; let middle_clock = middle.clock(); - if middle_clock <= clock { - if clock < middle_clock + middle.len() { - return Some(middle_index); - } + let middle_end = middle_clock.saturating_add(middle.len()); + + if clock < middle_clock { + right = middle_index; + } else if clock >= middle_end { left = middle_index + 1; } else { - right = middle_index - 1; + return Some(middle_index); } - middle_index = (left + right) / 2; } + None } @@ -347,7 +349,7 @@ impl DocStore { /// `doc.get_or_create_text("content")`) /// - [Parent::Id] for type as item (e.g `doc.create_text()`) /// - [None] means borrow left.parent or right.parent - pub fn repair(&mut self, item: &mut Item, store_ref: StoreRef) -> JwstCodecResult { + pub fn repair(&mut self, item: &mut Item, store_ref: StoreRef, pending_types: &PendingTypes) -> JwstCodecResult { if let Some(left_id) = item.origin_left_id { if let Node::Item(left_ref) = self.split_at_and_get_left(left_id)? { item.origin_left_id = left_ref.get().map(|left| left.last_id()); @@ -393,8 +395,12 @@ impl DocStore { } } _ => { - // GC & Skip are not valid parent, take it. - item.parent.take(); + if let Some(ty) = pending_types.get(parent_id) { + item.parent.replace(Parent::Type(ty.clone())); + } else { + // GC & Skip are not valid parent, take it. + item.parent.take(); + } } } } @@ -1126,6 +1132,50 @@ mod tests { assert_eq!(doc_store.get_node(Id::new(1, 35)), None); }); + + loom_model!({ + let mut failures: Vec<&'static str> = Vec::new(); + + { + let mut doc_store = DocStore::with_client(1); + let struct_info = Node::new_gc(Id::new(1, 0), 1); + doc_store.add_node(struct_info).unwrap(); + + let items = doc_store.items.get(&1).unwrap(); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| DocStore::get_node_index(items, 1))); + + match result { + Ok(index) if index.is_none() => {} + Ok(_) => failures.push("len=1 clock beyond end should return None"), + Err(_) => failures.push("len=1 clock beyond end should not panic"), + } + } + + { + let mut doc_store = DocStore::with_client(1); + let struct_info1 = Node::new_gc(Id::new(1, 0), 1); + let struct_info2 = Node::new_gc(Id::new(1, 1), 1); + doc_store.add_node(struct_info1).unwrap(); + doc_store.add_node(struct_info2).unwrap(); + + let items = doc_store.items.get(&1).unwrap(); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + DocStore::get_node_index(items, u64::MAX) + })); + + match result { + Ok(index) if index.is_none() => {} + Ok(_) => failures.push("huge clock should return None"), + Err(_) => failures.push("huge clock should not panic"), + } + } + + assert!( + failures.is_empty(), + "get_node_index edge cases failed: {}", + failures.join(", ") + ); + }); } #[test]