diff --git a/CHANGELOG.md b/CHANGELOG.md index 2529f10f..5d0816eb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ - Like the `invalidate` method, this method discards any cached value for the key, but returns a clone of the value. +### Fixed + +- Fixed the caches mutating a deque node through a `NonNull` pointer derived from a + shared reference. ([#259][gh-pull-0259]) + ### Removed - Removed `unsync` cache that was marked as deprecated in [v0.10.0](#version-0100). @@ -642,6 +647,7 @@ The minimum supported Rust version (MSRV) is now 1.51.0 (2021-03-25). [gh-issue-0034]: https://github.com/moka-rs/moka/issues/34/ [gh-issue-0031]: https://github.com/moka-rs/moka/issues/31/ +[gh-pull-0259]: https://github.com/moka-rs/moka/pull/259/ [gh-pull-0251]: https://github.com/moka-rs/moka/pull/251/ [gh-pull-0248]: https://github.com/moka-rs/moka/pull/248/ [gh-pull-0216]: https://github.com/moka-rs/moka/pull/216/ diff --git a/src/common/concurrent/deques.rs b/src/common/concurrent/deques.rs index e2c75267..d90ce48e 100644 --- a/src/common/concurrent/deques.rs +++ b/src/common/concurrent/deques.rs @@ -194,3 +194,5 @@ impl Deques { } } } + +// TODO: Add tests and run Miri with them. diff --git a/src/common/deque.rs b/src/common/deque.rs index 424fa921..d2022c5d 100644 --- a/src/common/deque.rs +++ b/src/common/deque.rs @@ -52,8 +52,8 @@ impl DeqNode { } } - pub(crate) fn next_node(&self) -> Option<&DeqNode> { - self.next.as_ref().map(|node| unsafe { node.as_ref() }) + pub(crate) fn next_node_ptr(this: NonNull) -> Option>> { + unsafe { this.as_ref() }.next } } @@ -126,11 +126,13 @@ impl Deque { } pub(crate) fn peek_front(&self) -> Option<&DeqNode> { - // This method takes care not to create mutable references to whole nodes, - // to maintain validity of aliasing pointers into `element`. self.head.as_ref().map(|node| unsafe { node.as_ref() }) } + pub(crate) fn peek_front_ptr(&self) -> Option>> { + self.head.as_ref().cloned() + } + /// Removes and returns the node at the front of the list. pub(crate) fn pop_front(&mut self) -> Option>> { // This method takes care not to create mutable references to whole nodes, @@ -158,8 +160,6 @@ impl Deque { } pub(crate) fn peek_back(&self) -> Option<&DeqNode> { - // This method takes care not to create mutable references to whole nodes, - // to maintain validity of aliasing pointers into `element`. self.tail.as_ref().map(|node| unsafe { node.as_ref() }) } @@ -221,6 +221,12 @@ impl Deque { } } + pub(crate) fn move_front_to_back(&mut self) { + if let Some(node) = self.head { + unsafe { self.move_to_back(node) }; + } + } + /// Unlinks the specified node from the current list. /// /// This method takes care not to create mutable references to `element`, to @@ -683,35 +689,67 @@ mod tests { // ------------------------------------------------------- // First iteration. // peek_front() -> node1 - let node1a = deque.peek_front().unwrap(); - assert_eq!(node1a.element, "a".to_string()); - let node2a = node1a.next_node().unwrap(); - assert_eq!(node2a.element, "b".to_string()); - let node3a = node2a.next_node().unwrap(); - assert_eq!(node3a.element, "c".to_string()); - assert!(node3a.next_node().is_none()); + let node1a = deque.peek_front_ptr().unwrap(); + assert_eq!(unsafe { node1a.as_ref() }.element, "a".to_string()); + let node2a = DeqNode::next_node_ptr(node1a).unwrap(); + assert_eq!(unsafe { node2a.as_ref() }.element, "b".to_string()); + let node3a = DeqNode::next_node_ptr(node2a).unwrap(); + assert_eq!(unsafe { node3a.as_ref() }.element, "c".to_string()); + assert!(DeqNode::next_node_ptr(node3a).is_none()); // ------------------------------------------------------- // Iterate after a move_to_back. // Move "b" to the back. So now "a" -> "c" -> "b". unsafe { deque.move_to_back(node2_ptr) }; - let node1a = deque.peek_front().unwrap(); - assert_eq!(node1a.element, "a".to_string()); - let node3a = node1a.next_node().unwrap(); - assert_eq!(node3a.element, "c".to_string()); - let node2a = node3a.next_node().unwrap(); - assert_eq!(node2a.element, "b".to_string()); - assert!(node2a.next_node().is_none()); + let node1a = deque.peek_front_ptr().unwrap(); + assert_eq!(unsafe { node1a.as_ref() }.element, "a".to_string()); + let node3a = DeqNode::next_node_ptr(node1a).unwrap(); + assert_eq!(unsafe { node3a.as_ref() }.element, "c".to_string()); + let node2a = DeqNode::next_node_ptr(node3a).unwrap(); + assert_eq!(unsafe { node2a.as_ref() }.element, "b".to_string()); + assert!(DeqNode::next_node_ptr(node2a).is_none()); // ------------------------------------------------------- // Iterate after an unlink. // Unlink the second node "c". Now "a" -> "c". unsafe { deque.unlink_and_drop(node3_ptr) }; - let node1a = deque.peek_front().unwrap(); - assert_eq!(node1a.element, "a".to_string()); - let node2a = node1a.next_node().unwrap(); - assert_eq!(node2a.element, "b".to_string()); - assert!(node2a.next_node().is_none()); + let node1a = deque.peek_front_ptr().unwrap(); + assert_eq!(unsafe { node1a.as_ref() }.element, "a".to_string()); + let node2a = DeqNode::next_node_ptr(node1a).unwrap(); + assert_eq!(unsafe { node2a.as_ref() }.element, "b".to_string()); + assert!(DeqNode::next_node_ptr(node2a).is_none()); + } + + #[test] + fn peek_and_move_to_back() { + let mut deque: Deque = Deque::new(MainProbation); + + let node1 = DeqNode::new("a".into()); + deque.push_back(Box::new(node1)); + let node2 = DeqNode::new("b".into()); + let _ = deque.push_back(Box::new(node2)); + let node3 = DeqNode::new("c".into()); + let _ = deque.push_back(Box::new(node3)); + // "a" -> "b" -> "c" + + let node1a = deque.peek_front_ptr().unwrap(); + assert_eq!(unsafe { node1a.as_ref() }.element, "a".to_string()); + unsafe { deque.move_to_back(node1a) }; + // "b" -> "c" -> "a" + + let node2a = deque.peek_front_ptr().unwrap(); + assert_eq!(unsafe { node2a.as_ref() }.element, "b".to_string()); + + let node3a = DeqNode::next_node_ptr(node2a).unwrap(); + assert_eq!(unsafe { node3a.as_ref() }.element, "c".to_string()); + unsafe { deque.move_to_back(node3a) }; + // "b" -> "a" -> "c" + + deque.move_front_to_back(); + // "a" -> "c" -> "b" + + let node1b = deque.peek_front().unwrap(); + assert_eq!(node1b.element, "a".to_string()); } #[test] diff --git a/src/common/timer_wheel.rs b/src/common/timer_wheel.rs index 60126207..6badd648 100644 --- a/src/common/timer_wheel.rs +++ b/src/common/timer_wheel.rs @@ -311,43 +311,31 @@ impl TimerWheel { /// Returns a pointer to the timer event (cache entry) at the front of the queue. /// Returns `None` if the front node is a sentinel. fn pop_timer_node(&mut self, level: usize, index: usize) -> Option>>> { - if let Some(node) = self.wheels[level][index].peek_front() { + let deque = &mut self.wheels[level][index]; + if let Some(node) = deque.peek_front() { if node.element.is_sentinel() { return None; } } - self.wheels[level][index].pop_front() + deque.pop_front() } /// Reset the positions of the nodes in the queue at the given level and index. + /// When done, the sentinel is at the back of the queue. fn reset_timer_node_positions(&mut self, level: usize, index: usize) { - if let Some(node) = self.wheels[level][index].peek_back() { - if node.element.is_sentinel() { - // The sentinel is at the back of the queue. We are already set. - return; - } - } else { - panic!( - "BUG: The queue is empty. level: {}, index: {}", - level, index - ) - } + let deque = &mut self.wheels[level][index]; + debug_assert!( + deque.len() > 0, + "BUG: The queue is empty. level: {}, index: {}", + level, + index + ); // Rotate the nodes in the queue until we see the sentinel at the back of the // queue. - loop { - // Safe to unwrap because we already checked the queue is not empty. - let node = self.wheels[level][index].pop_front().unwrap(); - let is_sentinel = node.element.is_sentinel(); - - // Move the front node to the back. - self.wheels[level][index].push_back(node); - - // If the node we just moved was the sentinel, we are done. - if is_sentinel { - break; - } + while !deque.peek_back().unwrap().element.is_sentinel() { + deque.move_front_to_back(); } } diff --git a/src/sync_base/base_cache.rs b/src/sync_base/base_cache.rs index 097526a0..7747ecb5 100644 --- a/src/sync_base/base_cache.rs +++ b/src/sync_base/base_cache.rs @@ -1682,10 +1682,6 @@ where // Move the skipped nodes to the back of the deque. We do not unlink (drop) // them because ValueEntries in the write op queue should be pointing them. - // - // TODO FIXME: This `move_to_back()` will be considered UB as violating the - // aliasing rule because these skipped nodes were acquired by `peek_front` or - // `next_node`. (They both return `&node` instead of `&mut node`). for node in skipped_nodes { unsafe { deqs.probation.move_to_back(node) }; } @@ -1723,7 +1719,7 @@ where let mut skipped_nodes = SmallVec::default(); // Get first potential victim at the LRU position. - let mut next_victim = deqs.probation.peek_front(); + let mut next_victim = deqs.probation.peek_front_ptr(); // Aggregate potential victims. while victims.policy_weight < candidate.policy_weight { @@ -1731,18 +1727,18 @@ where break; } if let Some(victim) = next_victim.take() { - next_victim = victim.next_node(); - let vic_elem = &victim.element; + next_victim = DeqNode::next_node_ptr(victim); + let vic_elem = &unsafe { victim.as_ref() }.element; if let Some(vic_entry) = cache.get(vic_elem.hash(), |k| k == vic_elem.key()) { victims.add_policy_weight(vic_entry.policy_weight()); victims.add_frequency(freq, vic_elem.hash()); - victim_nodes.push(NonNull::from(victim)); + victim_nodes.push(victim); retries = 0; } else { // Could not get the victim from the cache (hash map). Skip this node // as its ValueEntry might have been invalidated. - skipped_nodes.push(NonNull::from(victim)); + skipped_nodes.push(victim); retries += 1; if retries > MAX_CONSECUTIVE_RETRIES { @@ -2085,14 +2081,7 @@ where // invalidated ValueEntry (which should be still in the write op queue) // has a pointer to this node, move the node to the back of the deque // instead of popping (dropping) it. - // - // TODO FIXME: This `peek_front()` and `move_to_back()` combo will be - // considered UB as violating the aliasing rule. (`peek_front` returns - // `&node` instead of `&mut node`). - if let Some(node) = deq.peek_front() { - let node = NonNull::from(node); - unsafe { deq.move_to_back(node) }; - } + deq.move_front_to_back(); true } } @@ -2157,14 +2146,7 @@ where // invalidated ValueEntry (which should be still in the write op // queue) has a pointer to this node, move the node to the back of // the deque instead of popping (dropping) it. - // - // TODO FIXME: This `peek_front()` and `move_to_back()` combo will be - // considered UB as violating the aliasing rule (`peek_front` returns - // `&node` instead of `&mut node`). - if let Some(node) = deqs.write_order.peek_front() { - let node = NonNull::from(node); - unsafe { deqs.write_order.move_to_back(node) }; - } + deqs.write_order.move_front_to_back(); } } }