Repository: paritytech/parity-db (language: rust)

Issue:
Iteration behavior changes depending on the log worker thread work
In the following test, the presence or abscence of the `db.process_commits` call changes the iteration behavior of the last `iter.prev()` call.

If `db.process_commit` is never called, the last `iter.prev()` returns the key `[0]` as exepected, if `db.process_commit` is called before iterating, it returns `None`.

This test is doing both forward and backward iterations, I don't know if we want to support both on the same iterator...

```rust
	#[cfg(feature = "instrumentation")]
	#[test]
	fn test_process_commit_in_the_middle_of_iter() {
		let tmp = tempdir().unwrap();
		let mut options = Options::with_columns(tmp.path(), 1);
		options.columns[0].btree_index = true;
		options.always_flush = true;
		options.with_background_thread = false;

		let db = Db::open_or_create(&options).unwrap();
		db.commit::<_, Vec<u8>>(vec![(0, vec![0], Some(vec![0]))]).unwrap();
		db.process_commits().unwrap();
		let mut iter = db.iter(0).unwrap();
		iter.seek_to_last().unwrap();
		assert_eq!(iter.prev().unwrap(), Some((vec![0], vec![0]))); // State = 0
		assert_eq!(iter.next().unwrap(), None); // State = End
		assert_eq!(iter.prev().unwrap(), Some((vec![0], vec![0]))); // Fails because is None, work properly if process_commit is not called
	}
```

Changelog: 2022-10-24: Updated the bug description, the bug happens even if `process_commits` is called before iterating.


Discussion / hints:

Fix patch:
diff --git a/src/btree/iter.rs b/src/btree/iter.rs
index 32372c96..ef9c8ebb 100644
--- a/src/btree/iter.rs
+++ b/src/btree/iter.rs
@@ -276,10 +276,7 @@ impl BTreeIterState {
 	}
 
 	fn exit(&mut self, direction: IterDirection) -> bool {
-		loop {
-			if self.state.len() < 2 {
-				return true
-			}
+		while !self.state.is_empty() {
 			self.state.pop();
 			if let Some((ix, node)) = self.state.last_mut() {
 				debug_assert!(matches!(ix, LastIndex::Descend(_)));
@@ -291,14 +288,13 @@ impl BTreeIterState {
 							continue,
 						_ => LastIndex::Before(*child),
 					};
-					break
 				} else {
 					self.state.clear(); // should actually be unreachable
-					break
 				}
+				return false
 			}
 		}
-		false
+		true
 	}
 
 	pub fn next(
diff --git a/src/db.rs b/src/db.rs
index 228e9fc3..7e5be665 100644
--- a/src/db.rs
+++ b/src/db.rs
@@ -1760,6 +1760,10 @@ mod tests {
 		iter.seek_to_first().unwrap();
 		assert_eq!(iter.next().unwrap(), Some((key1.clone(), b"value1".to_vec())));
 		assert_eq!(iter.next().unwrap(), None);
+		assert_eq!(iter.prev().unwrap(), Some((key1.clone(), b"value1".to_vec())));
+		assert_eq!(iter.prev().unwrap(), None);
+		assert_eq!(iter.next().unwrap(), Some((key1.clone(), b"value1".to_vec())));
+		assert_eq!(iter.next().unwrap(), None);
 
 		iter.seek_to_first().unwrap();
 		assert_eq!(iter.next().unwrap(), Some((key1.clone(), b"value1".to_vec())));


Test patch: