Skip to content

Commit 648bfc5

Browse files
committed
ignore concurrency test in cache store file for miri (blocking not supported by miri)
1 parent a6d3986 commit 648bfc5

1 file changed

Lines changed: 28 additions & 10 deletions

File tree

cot/src/cache/store/file.rs

Lines changed: 28 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ use std::path::Path;
4747

4848
use blake3::hash;
4949
use chrono::{DateTime, Utc};
50+
use cot_core::error::impl_into_cot_error;
5051
use fs4::tokio::AsyncFileExt;
5152
use serde_json::Value;
5253
use thiserror::Error;
@@ -56,7 +57,6 @@ use tokio::task::spawn_blocking;
5657

5758
use crate::cache::store::{CacheStore, CacheStoreError, CacheStoreResult};
5859
use crate::config::Timeout;
59-
use cot_core::error::impl_into_cot_error;
6060

6161
const ERROR_PREFIX: &str = "file-based cache store error:";
6262
const TEMPFILE_SUFFIX: &str = "tmp";
@@ -203,11 +203,18 @@ impl FileStore {
203203
.await?;
204204

205205
// Here's the flow:
206-
// We sync the data after writing to it. This ensures that we get the correctly written data even if the later ops fail.
207-
// We unlock first to let progress move. This is free when there are no concurrent accessor. If we rename then unlock, the final file gets locked instead when subsequent readers progress, defeating the purpose of this method.
208-
// We create a new file handle to see whether the file still exists or not. If it doesn't exist, we move on. It's most likely has been deleted by `create_dir_root_sync` or `clear`.
209-
// Finally, we rename only if the thread reasonably hold the "last" lock to the .tmp.
210-
// This doesn't guarantee that no .tmp file will be renamed by another thread, but it pushes the guarantees towards "writers write and hold in .tmp file, not contesting the final file"
206+
// We sync the data after writing to it. This ensures that we get the correctly
207+
// written data even if the later ops fail. We unlock first to let
208+
// progress move. This is free when there are no concurrent accessor. If we
209+
// rename then unlock, the final file gets locked instead when subsequent
210+
// readers progress, defeating the purpose of this method. We create a
211+
// new file handle to see whether the file still exists or not. If it doesn't
212+
// exist, we move on. It's most likely has been deleted by
213+
// `create_dir_root_sync` or `clear`. Finally, we rename only if the
214+
// thread reasonably hold the "last" lock to the .tmp. This doesn't
215+
// guarantee that no .tmp file will be renamed by another thread, but it pushes
216+
// the guarantees towards "writers write and hold in .tmp file, not contesting
217+
// the final file"
211218
file.sync_data()
212219
.await
213220
.map_err(|e| FileCacheStoreError::Io(Box::new(e)))?;
@@ -368,13 +375,19 @@ impl FileStore {
368375
) -> CacheStoreResult<(tokio::fs::File, std::path::PathBuf)> {
369376
let temp_path = self.dir_path.join(format!("{key_hash}.{TEMPFILE_SUFFIX}"));
370377

371-
// We must let the loop to propagate upwards to catch sudden missing cache directory
372-
// Then it would be easier for us to wait for file creation where we offload one lock check into the OS by using `create_new()`. So, the flow looks like this,
378+
// We must let the loop to propagate upwards to catch sudden missing cache
379+
// directory Then it would be easier for us to wait for file creation
380+
// where we offload one lock check into the OS by using `create_new()`. So, the
381+
// flow looks like this,
373382
// 1. `create_new()` -> fail, we check if the error is AlreadyExists.
374-
// 2. In a condition where (1) is triggered, we park task into a blocking thread waiting for the lock to move.
383+
// 2. In a condition where (1) is triggered, we park task into a blocking thread
384+
// waiting for the lock to move.
375385
// 3. The blocking thread will only wait for the existing file in the temp_path
376386
//
377-
// This approach was chosen because we can't possibly (at least for now) to create a file AND lock that file atomically. A window where the existing file may get renamed or deleted is expected. Therefore, the blocking task is a pessimistic write.
387+
// This approach was chosen because we can't possibly (at least for now) to
388+
// create a file AND lock that file atomically. A window where the existing file
389+
// may get renamed or deleted is expected. Therefore, the blocking task is a
390+
// pessimistic write.
378391

379392
let mut retry_count = 0;
380393
let temp_file = loop {
@@ -755,6 +768,9 @@ mod tests {
755768
let _ = tokio::fs::remove_dir_all(&path).await;
756769
}
757770

771+
// Ignored in miri since it currently doesn't support
772+
// blocking lock operation
773+
#[cfg_attr(miri, ignore)]
758774
#[cot::test]
759775
async fn test_interference_during_write() {
760776
let path = make_store_path();
@@ -780,6 +796,7 @@ mod tests {
780796
let _ = tokio::fs::remove_dir_all(&path).await;
781797
}
782798

799+
#[cfg_attr(miri, ignore)]
783800
#[cot::test]
784801
async fn test_clear_during_write() {
785802
let path = make_store_path();
@@ -806,6 +823,7 @@ mod tests {
806823
let _ = tokio::fs::remove_dir_all(&path).await;
807824
}
808825

826+
#[cfg_attr(miri, ignore)]
809827
#[cot::test]
810828
async fn test_thundering_write() {
811829
let path = make_store_path();

0 commit comments

Comments
 (0)