Skip to content

Commit 495162f

Browse files
committed
Remove secondary check for duplicate blocks
cargo-mutants highlights that this isn't tested, and indeed it's pretty hard to test because it can only show up in a race between two writers. Because I'm planning to have a lock on writers, we should never need this.
1 parent 4fc89f1 commit 495162f

File tree

1 file changed

+34
-3
lines changed

1 file changed

+34
-3
lines changed

src/blockdir.rs

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -140,10 +140,11 @@ impl BlockDir {
140140
.await
141141
{
142142
Ok(()) => {}
143-
Err(err) if err.kind() == transport::ErrorKind::AlreadyExists => {
144-
// let's assume the contents are correct
145-
}
146143
Err(err) => {
144+
// We previously checked that the block was not already present. If there is another
145+
// backup concurrently writing, we might race with it and find here that the block's
146+
// already been written. However, I'm going to move towards holding a lock for all
147+
// writes, and then that will never happen.
147148
warn!(?err, ?hash, "Error writing block");
148149
return Err(err.into());
149150
}
@@ -483,6 +484,36 @@ mod test {
483484
assert_eq!(monitor.get_counter(Counter::BlockExistenceCacheMiss), 1);
484485
}
485486

487+
#[tokio::test]
488+
async fn store_existing_block_is_not_an_error() {
489+
let transport = Transport::temp();
490+
let blockdir = BlockDir::open(transport.clone());
491+
let mut stats = BackupStats::default();
492+
let monitor = TestMonitor::arc();
493+
let content = Bytes::from("stuff");
494+
let hash = blockdir
495+
.store_or_deduplicate(content.clone(), &mut stats, monitor.clone())
496+
.await
497+
.unwrap();
498+
assert_eq!(monitor.get_counter(Counter::BlockWrites), 1);
499+
assert_eq!(monitor.get_counter(Counter::DeduplicatedBlocks), 0);
500+
assert_eq!(monitor.get_counter(Counter::BlockExistenceCacheMiss), 1);
501+
assert!(blockdir.contains(&hash, monitor.clone()).await.unwrap());
502+
assert_eq!(monitor.get_counter(Counter::BlockExistenceCacheMiss), 1);
503+
assert_eq!(monitor.get_counter(Counter::BlockExistenceCacheHit), 1); // Since we just wrote it, we know it's there.
504+
505+
// Open again to get a fresh cache
506+
let blockdir = BlockDir::open(transport.clone());
507+
let monitor = TestMonitor::arc();
508+
let _hash = blockdir
509+
.store_or_deduplicate(content.clone(), &mut stats, monitor.clone())
510+
.await
511+
.unwrap();
512+
assert_eq!(monitor.get_counter(Counter::BlockWrites), 0);
513+
assert_eq!(monitor.get_counter(Counter::DeduplicatedBlocks), 1);
514+
assert_eq!(monitor.get_counter(Counter::BlockExistenceCacheMiss), 1);
515+
}
516+
486517
#[tokio::test]
487518
async fn blocks_async() {
488519
let tempdir = TempDir::new().unwrap();

0 commit comments

Comments
 (0)